Validate S-102 grid dimension metadata before allocating - #46
Open
SemanticWave-Hoyeon wants to merge 1 commit into
Open
SemanticWave-Hoyeon wants to merge 1 commit into
SemanticWave-Hoyeon wants to merge 1 commit into
Conversation
S102_VG_BathymetryCoverage::Read() sized its buffer with numLat * numLon * 2 evaluated in int. Both operands come from the numPointsLatitudinal / numPointsLongitudinal attributes of the file being opened, so a product above INT_MAX wraps and the allocation ends up smaller than the dataset that H5Dread(H5S_ALL) subsequently writes into it. Nothing coupled the size computed from the attributes to the size HDF5 takes from the dataset's own dataspace, so a file whose attributes disagree with its dataspace reached the same result without any wrap. Read() now rejects non-positive dimensions, multiplies with overflow reported instead of wrapped, bounds the resulting point count, requires the compound element width that the unpacking loop assumes, and compares the declared dimensions against the dataspace H5Dread will actually use. Allocation failure is reported rather than thrown, and the type, dataspace and dataset handles are released on every path. Two follow-on problems are fixed with it. S102_FI_BathymetryCoverage::Read() discarded the return value and returned true unconditionally, leaving callers to walk depth[] and uncertainty[] after a read that never populated them; it now propagates the result and closes the group handle. S102H5::CreateBitmap() and S102H5::SetPositive() recomputed width * height in int and indexed depth[] with the result; both now iterate over the count that Read() recorded and return early when no grid was read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
S102_VG_BathymetryCoverage::Read()computed its buffer size asnumLat * numLon * 2inint. Both operands are read from thenumPointsLatitudinal/numPointsLongitudinalattributes of the file being opened, so a product aboveINT_MAXwraps and the allocation ends up smaller than the dataset thatH5Dread(..., H5S_ALL, ...)subsequently writes into it.There is a second route to the same outcome that does not involve the arithmetic at all.
H5S_ALLmakes HDF5 read the whole dataset as described by the file's own dataspace, and that dataspace was never compared against the attribute values the allocation was sized from. Attributes that simply disagree with the stored dataspace produce the same undersized buffer.The change
Read()now performs, in order:H5Dreadwill actually use.Allocation failure is reported rather than thrown, and the type, dataspace and dataset handles are released on every exit path.
One note for reviewers on step 3: the bound is
100,000,000points, which is roughly 270x the ~600x600 grid that S-102 11.2.2 suggests, so legitimate high-resolution products are unaffected. At that bound a single coverage can still ask for about 1.6 GB across the three buffers, so please lower it if the target hardware has a tighter budget. It is a named constant for that reason.Two follow-on problems are fixed along with it, because the validation is not much use while they remain:
S102_FI_BathymetryCoverage::Read()discarded the return value and returnedtrueunconditionally, which left callers walkingdepth[]anduncertainty[]after a read that never populated them. It now propagates the result, and closes the group handle it opens.S102H5::CreateBitmap()andS102H5::SetPositive()recomputedwidth * heightinintand indexeddepth[]with the result, repeating the original pattern in two more places. Both now iterate over the countRead()recorded, and return early when no grid was read.S102_VG_BathymetryCoveragegained apointCountReadmember to make that count available.What was and was not verified
The validation logic was extracted and exercised standalone against a benign 600x600 grid, dimension pairs whose 32-bit product wraps, attribute/dataspace mismatches, non-positive and oversized dimensions, non-compound and unexpected-width element types, a non-2D dataspace, and a failing
H5Dopen. The benign grid is accepted and the read fits its allocation; the rest are refused before anything is allocated. Under AddressSanitizer the same harness reproduces the out-of-bounds write on the current code and reports none on this branch.I could not build the full project: it needs MSVC, MFC and the Windows HDF5 build, and I do not have that toolchain here. So this has not been compiled as part of
GISLibrary, and it has not been run against real S-102 products or exercised through the UI. Both are worth doing before merge, and I am happy to adjust anything the build turns up.