Skip to content

Return pos 0 instead of aborting when IFields data is truncated - #1050

Merged
copybara-service[bot] merged 1 commit into
google:devfrom
Zhuoxi2000:fix-fields-truncated-read-abort
Oct 7, 2026
Merged

copybara-service[bot] merged 1 commit into
google:devfrom
Zhuoxi2000:fix-fields-truncated-read-abort

Conversation

@Zhuoxi2000

Copy link
Copy Markdown

Fixes #1048

What / why

ReadVisitor::operator()(IFields&) in io/fields.cc pushes onto end_, but it returns without popping when pos + num_u32 > span.size(). As a result, ~ReadVisitor fails HWY_ASSERT(end_.empty()) and the process aborts. ReadResult in io/fields.h says this case should return pos == 0 instead ("the span is shorter than the data says it should be"). Because of the abort, the model_store.cc diagnostics for corrupt configs and MatPtr blobs ("Error deserializing config", "Deserializing MatPtr %s failed") are never printed.

This PR pops end_ in that branch, the same way the SkipField() branch just above does. It also adds FieldsTest.TestTruncatedData, which reads a span that is shorter than the stored size and expects pos == 0.

Callers that already abort on pos == 0 behave the same as before, except that they now print their own error message.

Testing

cmake --preset make -DCMAKE_BUILD_TYPE=Release -DGEMMA_ENABLE_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5
cmake --build build -j8 --target fields_test blob_store_test
./build/fields_test
  • Before (new test only, io/fields.cc from dev @ 0224dcb): aborts with SIGABRT (exit code 134).
    [ RUN      ] FieldsTest.TestTruncatedData
    Invalid IFields: pos 1 + num_u32 112 > size 2
    Abort at fields.cc:110: Assert end_.empty():
    
  • After: [ PASSED ] 10 tests.
  • blob_store_test: 2/2 passed. gemma/configs_test.cc is Bazel-only; built separately against the CMake build, it passes 9/9.
  • clang-format (repo .clang-format) reports no changes to io/fields.cc or to the new test.

Environment: macOS 26.5, Apple M4, Apple clang 17.0.0, CMake 4.4.3.

ReadVisitor::operator()(IFields&) pushes onto end_, but the branch for
`pos + num_u32 > span.size()` returned without popping. ~ReadVisitor
then failed HWY_ASSERT(end_.empty()), so reading a truncated or corrupt
config or MatPtr aborted instead of returning ReadResult.pos == 0 as
documented in fields.h. Callers such as model_store.cc never got to
print their "Error deserializing config" diagnostics.

Pop end_ in that branch, as the SkipField branch above already does,
and add a test that reads a span shorter than the stored size.

@jan-wassenberg jan-wassenberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@jan-wassenberg jan-wassenberg added the copybara-import Trigger Copybara for merging pull requests label Oct 6, 2026
@copybara-service
copybara-service Bot merged commit f7b787a into google:dev Oct 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copybara-import Trigger Copybara for merging pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants