Hacktoberfest 2026: die Issues, die Maintainer für den Oktober markiert haben – offen und einsteigerfreundlich. Hacktoberfest-Issues durchsuchen

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

Offen
  • Stimmt etwas nicht?
#1,048 2 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
1/5
Geschätzter Aufwand
Unter einer Stunde
Anfängerfreundlichkeit
25/100
Issue-Typ
Bug
Klarheit
Klar beschrieben
Aktivitätsstatus
Aktiv
Tech-Stack
cpp
Bereich
data

Rechercherichtung

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.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

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.

Vorherrschende Sprache
C++
Sterne
7.1k
Forks
668
Ø Merge
19 Std. 56 Min.
Gemergte PRs (30 T.)
17

Entwicklungsumgebung

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus google/gemma.cpp

Alle Issues in google/gemma.cpp

Ähnliche Issues

Weitere Issues zu C++

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.