Skip to content

fix ScanTable causing assertion failure in Encode - #1344

Open
yvonnelxxxx wants to merge 1 commit into
google:mainfrom
yvonnelxxxx:fix/encode-assert
Open

yvonnelxxxx wants to merge 1 commit into
google:mainfrom
yvonnelxxxx:fix/encode-assert

Conversation

@yvonnelxxxx

Copy link
Copy Markdown

This PR attempts to fix #1343

Problem

Repairer::ScanTable() in db/repair.cc scans a table to derive its smallest/largest InternalKeys. When a corrupted table opens successfully (iterator status OK) but every key fails ParseInternalKey(), the empty flag stays true and t.meta.smallest / t.meta.largest are never populated — they remain default-constructed (empty rep_).

Despite that, the old code pushed the table straight into tables_:

if (status.ok()) {
  tables_.push_back(t);   // pushed with empty smallest/largest!
} else {
  RepairTable(fname, t);
}

Later, WriteDescriptor() calls edit_.AddFile(0, ..., t.meta.smallest, t.meta.largest) for every entry, and VersionEdit::EncodeTo() (db/version_edit.cc:82) serializes them via f.smallest.Encode() / f.largest.Encode(). That hits:

assert(!rep_.empty())

at db/dbformat.h:150 and aborts:

leveldb: .../db/dbformat.h:150: Slice leveldb::InternalKey::Encode() const:
Assertion `!rep_.empty()' failed.
Aborted

Stack:

#0 __assert_fail (!rep_.empty())                          db/dbformat.h:150
#1 InternalKey::Encode()                                  db/dbformat.h:150
#2 VersionEdit::EncodeTo()  -> f.smallest.Encode()        db/version_edit.cc:82
#3 Repairer::WriteDescriptor()                            db/repair.cc:380
#4 Repairer::Run()                                        db/repair.cc:75
#5 RepairDB()                                             db/repair.cc:452

This is a realistic corruption scenario: zeroing the data-block region of an SST while leaving the index + footer intact lets Table::Open succeed, so the iterator is "OK" but yields no parseable keys.

Fix

In ScanTable(), gate the push on !empty. If the table opened but produced no valid keys, archive it instead of passing empty key bounds downstream to WriteDescriptor:

// db/repair.cc — end of ScanTable()

// Before
if (status.ok()) {
  tables_.push_back(t);
} else {
  RepairTable(fname, t);
}

// After
if (status.ok() && !empty) {
  tables_.push_back(t);
} else if (status.ok() && empty) {
  ArchiveFile(TableFileName(dbname_, number));
  Log(options_.info_log, "Table #%llu: dropped (no valid keys)",
      (unsigned long long)t.meta.number);
} else {
  RepairTable(fname, t);
}

This keeps the existing path for tables with valid keys unchanged, and routes keyless-but-OK tables through the same ArchiveFile mechanism already used for unreadable files (the "dropped" branch above). An alternative would be to weaken the assert(!rep_.empty()) in InternalKey::Encode(), but that would let empty key bounds be silently written into the manifest — a worse outcome than dropping the corrupt table, so archiving is preferred.

Verification

Sanitizer rebuild

Rebuilt the library with AddressSanitizer and UndefinedBehaviorSanitizer to confirm the assertion crash is gone and no memory/UB issues remain:

export CC=clang CXX=clang++ LD=clang++
export CFLAGS="-fsanitize=address,undefined -fno-sanitize=function \
  -fsanitize-address-use-after-scope -g -O0 -fstandalone-debug -w -fPIC \
  -fno-inline -fno-omit-frame-pointer -fno-optimize-sibling-calls \
  -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"
export CXXFLAGS="$CFLAGS" LDFLAGS="$CXXFLAGS"
cmake -S <leveldb-src> -B build-san -DBUILD_SHARED_LIBS=OFF \
  -DLEVELDB_BUILD_TESTS=OFF -DLEVELDB_BUILD_BENCHMARKS=OFF -DLEVELDB_INSTALL=ON \
  -DCMAKE_C_FLAGS="$CFLAGS" -DCMAKE_CXX_FLAGS="$CXXFLAGS"
cmake --build build-san -j && cmake --install build-san

Before fix:

poc: .../db/dbformat.h:150: Slice leveldb::InternalKey::Encode() const:
Assertion `!rep_.empty()' failed.
Aborted (exit 134)

After fix:

(clean exit, code 0 — no sanitizer errors)

Test-suite results

Ran the full LevelDB test suite under ASan + UBSan with halt_on_error=1:

ASAN_OPTIONS=halt_on_error=1 UBSAN_OPTIONS=halt_on_error=1 ctest --test-dir build-tests --output-on-failure
Test Result
leveldb_tests (211 tests) Passed (209 passed, 2 skipped — compression env-dependent)
c_test (incl. Test repair) Passed
env_posix_test (7 tests) Passed

All tests pass with the fix applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: RepairDB pushes tables with empty smallest/largest InternalKeys causing assertion failure in Encode

1 participant