Fix heap-buffer-overflow in GeorefMetadataLayerDescriptor #145
Open
oliverchang wants to merge 1 commit intoOpenNavigationSurface:masterfrom
Open
Fix heap-buffer-overflow in GeorefMetadataLayerDescriptor #145oliverchang wants to merge 1 commit intoOpenNavigationSurface:masterfrom
oliverchang wants to merge 1 commit intoOpenNavigationSurface:masterfrom
Conversation
A malformed HDF5 file could trigger an out-of-bounds read in HDF5's `H5HG_read`. This occurred when the `COMPOUND_RECORD_DEFINITION` attribute's physical storage size did not match the product of the number of fields and the datatype size (e.g., 360 bytes stored for 15 elements of 16 bytes each). The resulting incorrect stride caused HDF5 to interpret garbage data as Global Heap IDs for Variable Length (VL) strings. Dereferencing these invalid IDs led to the overflow. Validation has been added to ensure `attribute.getStorageSize() == numFields * attrDataType.getSize()` before calling `attribute.read()`. If a mismatch is detected, an `InvalidValueSize` exception is thrown. This is caught in `BAG::Dataset::readDataset`, allowing the application to safely skip the malformed layer. Co-authored-by: CodeMender <codemender-patching@google.com> Fixes: OpenNavigationSurface#141
Collaborator
|
@oliverchang Thanks for submitting this PR. It looks like this is causing some tests to now fail. Can you address that? Thanks. |
selimnairb
reviewed
Feb 10, 2026
|
|
||
| const auto attrDataType = attribute.getDataType(); | ||
| if (attribute.getStorageSize() != numFields * attrDataType.getSize()) | ||
| throw InvalidValueSize{}; |
Collaborator
There was a problem hiding this comment.
Please use curly braces, even for a single-line if statement, or place on a single line (yes, I know, other parts of the codebase don't do this, but I want to make sure new code does).
Author
|
Thank you! It looks like I'm having a fair bit of trouble getting tests running properly though (pre-existing failures with and without ASan).. so it might take a while.. |
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
2 participants
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.
A malformed HDF5 file could trigger an out-of-bounds read in HDF5's
H5HG_read. This occurred when theCOMPOUND_RECORD_DEFINITIONattribute's physical storage size did not match the product of the number of fields and the datatype size (e.g., 360 bytes stored for 15 elements of 16 bytes each).The resulting incorrect stride caused HDF5 to interpret garbage data as Global Heap IDs for Variable Length (VL) strings. Dereferencing these invalid IDs led to the overflow.
Validation has been added to ensure
attribute.getStorageSize() == numFields * attrDataType.getSize()before callingattribute.read(). If a mismatch is detected, anInvalidValueSizeexception is thrown. This is caught inBAG::Dataset::readDataset, allowing the application to safely skip the malformed layer.Fixes: #141