Repository navigation
Harden BinaryAnnotator bounds checks and malformed-schema stability - #9444
Open
professor-moody wants to merge 1 commit into
Open
professor-moody wants to merge 1 commit into
professor-moody wants to merge 1 commit into
Conversation
### Summary Add defensive bounds checks and malformed-schema stability validation to `BinaryAnnotator` in `src/binary_annotator.h` and `src/binary_annotator.cpp`, while preserving normal annotation layouts. - Handle empty region lists before accessing the last region in `FixMissingSections()` and `ContainsSection()`. - Keep inline struct traversal state inside `BuildStruct()` entry and exit. An active-object stack stops cyclic traversal, and a scope guard restores that stack on exit. A per-root stop flag prevents further descent after a cycle, exhausted input, or a non-progressing child. The fixed-array call block and function signatures remain unchanged. - Validate `Vector64` byte spans using remaining bytes and division, and preserve the vector section type when reporting an invalid length. - Read union type-vector offsets using the field's 32-bit or 64-bit width, validate the resulting data position, and advance the element slot when recording an invalid union-vector element. ### Validation `BinaryAnnotatorBoundsTest()`, `BinaryAnnotatorEmptySectionTest()`, and `BinaryAnnotatorUnionOffset64Test()` in `tests/fuzz_test.cpp` run through the existing `flattests` suite. CMake and Bazel test wiring include the annotator implementation. - Positive controls cover valid `Vector64`, empty vectors, a verified union vector, nested inline arrays, sibling traversal, and repeated annotation calls on the same instance. - Existing malformed-schema and buffer regressions cover cyclic structs, empty sections, vector bounds, and union-element bookkeeping. A small truncated nested-array control checks that annotation growth stops when its data ends. - The focused empty-section test checks that a later string is still annotated after an empty union section, including a repeated annotation call. The 64-bit union test checks the eight-byte type-vector prefix, an out-of-range full-width offset whose low 32 bits are in range, and a truncated eight-byte companion offset. - Temporary counters confirmed that the empty-region lookup and 64-bit union offset paths execute. All counters were removed before the final builds. - The patched full suite prints `ALL TESTS PASSED` and exits 0 under AddressSanitizer and UndefinedBehaviorSanitizer with ASAN_OPTIONS=detect_leaks=0 UBSAN_OPTIONS=halt_on_error=1. Leak detection is disabled for the unchanged cross-namespace fixture documented by the prior independent review; address and undefined behavior checks remain enabled. - The patched Release suite exits 0. The existing annotation generator exits 0 in both builds and leaves all 24 tracked `.afb` files unchanged: 23 regenerated outputs and one historical file. - Changed C++ ranges have zero clang-format replacements and added lines are at most 80 columns. Bazel wiring was inspected but Bazel was not executed. The changes do not alter the wire format or public function signatures. Inline struct traversal has no new fixed-depth cutoff.
This branch has not been deployed
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.
Summary
Add defensive bounds checks and malformed-schema stability validation to
BinaryAnnotatorinsrc/binary_annotator.handsrc/binary_annotator.cpp, while preserving normal annotation layouts.FixMissingSections()andContainsSection().BuildStruct()entry and exit. An active-object stack stops cyclic traversal, and a scope guard restores that stack on exit. A per-root stop flag prevents further descent after a cycle, exhausted input, or a non-progressing child. The fixed-array call block and function signatures remain unchanged.Vector64byte spans using remaining bytes and division, and preserve the vector section type when reporting an invalid length.Validation
BinaryAnnotatorBoundsTest(),BinaryAnnotatorEmptySectionTest(), andBinaryAnnotatorUnionOffset64Test()intests/fuzz_test.cpprun through the existingflattestssuite. CMake and Bazel test wiring include the annotator implementation.Vector64, empty vectors, a verified union vector, nested inline arrays, sibling traversal, and repeated annotation calls on the same instance.ALL TESTS PASSEDand exits 0 under AddressSanitizer and UndefinedBehaviorSanitizer with ASAN_OPTIONS=detect_leaks=0 UBSAN_OPTIONS=halt_on_error=1. Leak detection is disabled for the unchanged cross-namespace fixture documented by the prior independent review; address and undefined behavior checks remain enabled..afbfiles unchanged: 23 regenerated outputs and one historical file.The changes do not alter the wire format or public function signatures. Inline struct traversal has no new fixed-depth cutoff.