Use FlatTensorHeader::kHeaderOffset for the FlatTensor size check - #22137
Open
shoumikhin wants to merge 1 commit into
Open
Use FlatTensorHeader::kHeaderOffset for the FlatTensor size check#22137shoumikhin wants to merge 1 commit into
shoumikhin wants to merge 1 commit into
Conversation
The minimum size check in `FlatTensorDataMap::load` spelled the size of the flatbuffer header as `sizeof(flatbuffers::uoffset_t)` plus `flatbuffers::kFileIdentifierLength`. `kFileIdentifierLength` only exists at namespace scope in newer flatbuffers releases. In flatbuffers 1.12 the same constant exists only as a member of `FlatBufferBuilder`, so a build against an older flatbuffers fails to compile: ``` error: no member named 'kFileIdentifierLength' in namespace 'flatbuffers' ``` The number being computed is 8: a 4 byte root table offset followed by a 4 byte file identifier. `FlatTensorHeader::kHeaderOffset` already names exactly that number, because the FlatTensor header starts right after the flatbuffer header ends. The equivalent bounds check in `Program::load` already uses `ExtendedHeader::kHeaderOffset` for the same reason. Use `FlatTensorHeader::kHeaderOffset` here too. The bound does not move, behavior is unchanged, and no new include is needed because `flat_tensor_header.h` was already included.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22137
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 PendingAs of commit 3b175d0 with merge base 88a5f60 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
added
the
release notes: runtime
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
FlatTensorDataMap::loadchecks that the buffer is at least as long as the flatbuffer header before it reads the root table offset out of it. It computed that length assizeof(flatbuffers::uoffset_t) + flatbuffers::kFileIdentifierLength.kFileIdentifierLengthonly exists at namespace scope in newer flatbuffers releases. In flatbuffers 1.12 the same constant exists only as a member ofFlatBufferBuilder, so a build against an older flatbuffers fails to compile:The submodule pinned in this repository is v24.3.25, where the namespace scope name does exist, so CI here cannot see the problem.
What this changes
The number being computed is 8: a 4 byte root table offset followed by a 4 byte file identifier.
FlatTensorHeader::kHeaderOffsetalready names exactly that number, because the FlatTensor header begins right after the flatbuffer header ends. The matching bounds check inProgram::loadalready usesExtendedHeader::kHeaderOffsetfor the same reason.So this swaps the expression for
FlatTensorHeader::kHeaderOffset. The bound does not move, behavior is unchanged, and no include is added,flat_tensor_header.hwas already included by this file.Test plan
Compiled
extension/flat_tensor/flat_tensor_data_map.cppwith-std=c++17 -Wallagainst the pinned flatbuffers v24.3.25. Clean, no warnings and no errors.Checked that the fix is load bearing by compiling both spellings against both flatbuffers versions:
The probe also
static_asserts that the new expression equals the old one and equals 8, so the two spellings really do produce the same bound.