Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

IFields::Read aborts on truncated data instead of returning pos == 0

Aperta
  • Qualcosa non va?
#1,048 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

@Zhuoxi2000 ci sta già lavorando.

Dal 6/10/2026.

  • #1050 di @Zhuoxi2000 — aperta

Valutazione

Difficoltà
1/5
Tempo stimato
Meno di un'ora
Idoneità per principianti
25/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
cpp
Ambito
data

Direzione di ricerca

The unbalanced push/pop is in ReadVisitor::operator()(IFields&) in io/fields.cc, where the truncation branch returns before ~ReadVisitor's HWY_ASSERT(end_.empty()) runs; the SkipField branch shows the pop that is missing. ReadResult's contract in io/fields.h documents pos == 0 for a short span, and gemma/model_store.cc's config and MatPtr checks are the affected callers. Add the TestTruncatedData case to io/fields_test.cc, build the fields_test target, and confirm the test plus the existing cases pass. Note the reporters state they already have the fix and test on a branch and will open the PR themselves.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

What happens

ReadVisitor::operator()(IFields&) in io/fields.cc (on dev, 8073ef9) begins by pushing onto end_. Then:

  • The SkipField() branch returns early and pops first ("undo push_back to keep the stack balanced").
  • The truncation branch does not pop:
if (HWY_UNLIKELY(result_.pos + num_u32 > span_.size())) {
  NotifyInvalid("Invalid IFields: pos %zu + num_u32 %u > size %zu\n",
                result_.pos, num_u32, span_.size());
  return;  // end_ is still non-empty
}

So when the stored size of an IFields is larger than the span, ~ReadVisitor fails HWY_ASSERT(end_.empty()); // Bug if push/pop are not balanced. and the process aborts. Read() never returns.

ReadResult in io/fields.h documents that pos is 0 "if there was an unrecoverable error: any field has an invalid value, or the span is shorter than the data says it should be". Other invalid inputs, such as an out-of-range enum, bool or float, already return pos == 0, so only this path is inconsistent. main (3ed403e) has the same code.

This affects gemma/model_store.cc, which checks result.pos after reading the config ("Error deserializing config") and each MatPtr ("Deserializing MatPtr %s failed (pos %zu of %zu)."). When an .sbs file is truncated or corrupt, users get the internal Assert end_.empty() in fields.cc instead of those messages. Both callers abort on pos == 0 anyway, so the main effect is the misleading diagnostic. Any other caller that wants to handle pos == 0 cannot do so.

Minimal repro

Add this test to io/fields_test.cc:

// If the stored size exceeds the span, Read must report the error via
// `pos == 0` (see `ReadResult`) instead of aborting.
TEST(FieldsTest, TestTruncatedData) {
  const NewFields new_fields = ModifiedNewFields();
  const std::vector<uint32_t> storage = new_fields.Write();
  ASSERT_GT(storage.size(), 2u);

  NewFields copy;
  const ReadResult result = copy.Read(Span(storage.data(), 2), 0);
  EXPECT_EQ(0, result.pos);
}

cmake -B build -DGEMMA_ENABLE_TESTS=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build -j4 --target fields_test
./build/fields_test --gtest_filter='*Truncated*'

Expected vs actual

Expected: Read returns a ReadResult with pos == 0, as documented in fields.h, and the test passes.

Actual: the process aborts (SIGABRT, exit code 134):

[ RUN      ] FieldsTest.TestTruncatedData
Invalid IFields: pos 1 + num_u32 112 > size 2
Abort at fields.cc:110: Assert end_.empty():

Environment

  • gemma.cpp dev @ 8073ef9 (also present on main @ 3ed403e)
  • macOS 26.5, Apple M4, Apple clang 17.0.0, CMake Release build

Suggested fix

Pop end_ before returning, the same way the SkipField() branch does:

     if (HWY_UNLIKELY(result_.pos + num_u32 > span_.size())) {
       NotifyInvalid("Invalid IFields: pos %zu + num_u32 %u > size %zu\n",
                     result_.pos, num_u32, span_.size());
+      end_.pop_back();  // keep the stack balanced, see `~ReadVisitor`
       return;
     }

With this change, the test above and the existing fields_test cases pass (10/10). configs_test (9/9) and blob_store_test (2/2) also pass.

We have this fix and the regression test ready on a branch and will open the PR against dev ourselves.

Lingua principale
C++
Stelle
7k
Fork
660
Merge medio
1g 15h
PR unite (30g)
19

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di google/gemma.cpp

Tutte le issue di google/gemma.cpp

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.