Hacktoberfest 2026: los issues que los mantenedores marcaron para octubre, abiertos y aptos para principiantes. Explorar issues de Hacktoberfest

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

Abierto
  • ¿Algo no está bien?
#1,048 2 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

@Zhuoxi2000 ya está trabajando en esto.

Desde el 6/10/2026.

  • #1050 de @Zhuoxi2000 — abierto

Evaluación

Dificultad
1/5
Tiempo estimado
Menos de una hora
Aptitud para principiantes
25/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
cpp
Área
data

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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.

Lenguaje dominante
C++
Estrellas
7k
Forks
660
Merge medio
1 d 15 h
PR fusionados (30 d)
19

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de google/gemma.cpp

Todos los issues de google/gemma.cpp

Issues similares

Más issues de C++

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.