google / google/leveldb

Bug: ChangeOptions lowers block_restart_interval without resetting BlockBuilder counter_ causing assertion failure

Open
#1,339 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
39.4k
Forks
8.2k
PR merge metrics
No merged PRs in 30d

Description

## Summary

`TableBuilder::ChangeOptions` updates `block_restart_interval` in the shared `Options` struct that `BlockBuilder` reads via pointer, but does not reset the data block's internal `counter_`. When the new interval is smaller than the current `counter_`, the next `BlockBuilder::Add` trips `assert(counter_ <= options_->block_restart_interval)` at `block_builder.cc:74` and aborts.

## Version

```
$ git describe --tags
1.23-91-g7ee830d
```

## Description

`BlockBuilder` stores a `const Options*` pointer (not a copy) to the `Options` member inside `TableBuilder::Rep`:

```cpp
// table/table_builder.cc:27
data_block(&options),
```

```cpp
// table/block_builder.h:44
const Options* options_;
```

`BlockBuilder::Add` uses `options_->block_restart_interval` to decide when to write restart points, and maintains an invariant `counter_ <= block_restart_interval`:

```cpp
// table/block_builder.cc:71-88
void BlockBuilder::Add(const Slice& key, const Slice& value) {
Slice last_key_piece(last_key_);
assert(!finished_);
assert(counter_ <= options_->block_restart_interval); // line 74 — crashes
// ...
if (counter_ < options_->block_restart_interval) {
// compute shared prefix with previous key
} else {
restarts_.push_back(buffer_.size()); // restart point
counter_ = 0; // reset
}
// ...
counter_++;
}
```

`TableBuilder::ChangeOptions` overwrites `rep_->options` in place, so the new `block_restart_interval` is immediately visible to `BlockBuilder` through the pointer alias — but `counter_` is left untouched:

```cpp
// table/table_builder.cc:78-92
Status TableBuilder::ChangeOptions(const Options& options) {
if (options.comparator != rep_->options.comparator) {
return Status::InvalidArgument("changing comparator while building table");
}
// Note that any live BlockBuilders point to rep_->options and therefore
// will automatically pick up the updated options.
rep_->options = options;
rep_->index_block_options = options;
rep_->index_block_options.block_restart_interval = 1;
return Status::OK();
}
```

The code comment at lines 86-87 acknowledges the pointer aliasing for *options*, but overlooks the fact that `BlockBuilder` has *other internal state* (`counter_`) that must remain consistent with those options. After `ChangeOptions` lowers the interval below the accumulated `counter_`, the invariant is silently broken. The next `Add` hits the assertion before the `else` branch (lines 84-88) can self-correct.

This is a library bug, not API misuse:

- `options.h:104` explicitly documents `block_restart_interval`: "This parameter can be changed dynamically."
- `table_builder.h:41-47` documents `ChangeOptions` as changing options used by the builder, returning an error only for fields that cannot change dynamically (comparator). `block_restart_interval` is not among the rejected fields, and the call returns `Status::OK()`.
- `ChangeOptions` has no `REQUIRES` clause restricting *when* it may be called (before/after `Add`, between `Flush`es, etc.).
- The assertion checks **internal state** (`counter_` vs `block_restart_interval`), not an input requirement.
- The `Add` call ordering satisfies all documented preconditions: keys are strictly increasing, and neither `Finish()` nor `Abandon()` has been called.

The `else` branch at `block_builder.cc:84-88` already handles `counter_ >= interval` correctly — it writes a restart point and resets `counter_`. The assertion is stronger than the code logic requires; `counter_ > interval` is simply a subset of the case the `else` branch already handles. Restart points are stored as explicit offsets and do not need to be at regular intervals, so an extra restart point from an overshot `counter_` is harmless to the SSTable format.

## PoC Code
```cpp
#include "leveldb/env.h"
#include "leveldb/options.h"
#include "leveldb/slice.h"
#include "leveldb/table_builder.h"
#include "helpers/memenv/memenv.h"

// ChangeOptions lowers block_restart_interval below the data block's current
// counter_ without resetting it; the next Add trips
// assert(counter_ <= options_->block_restart_interval) at block_builder.cc:74.

int main() {
leveldb::Env* env = leveldb::NewMemEnv(leveldb::Env::Default());
leveldb::WritableFile* f = nullptr;
env->NewWritableFile("/t", &f);

leveldb::Options o;
o.env = env;
o.block_restart_interval = 2;

leveldb::TableBuilder b(o, f);
b.Add("a", "v"); // counter_ -> 1
b.Add("b", "v"); // counter_ -> 2

leveldb::Options n = o;
n.block_restart_interval = 1;
b.ChangeOptions(n);

b.Add("c", "v"); // assert(2 <= 1) — CRASH
b.Abandon();
delete f;
delete env;
}
```

## Stack Trace
```
ERROR: AddressSanitizer: ABRT on unknown address 0x000000003ea0 (pc 0x7b537e5c0b2c bp 0x7ffca2274480 sp 0x7ffca2274440 T0)
#0 0x7b537e5c0b2c in __pthread_kill_implementation nptl/pthread_kill.c:44:76
#1 0x7b537e5c0b2c in __pthread_kill_internal nptl/pthread_kill.c:78:10
#2 0x7b537e5c0b2c in pthread_kill nptl/pthread_kill.c:89:10
#3 0x7b537e56727d in raise signal/../sysdeps/posix/raise.c:26:13
#4 0x7b537e54a8fe in abort stdlib/abort.c:79:7
#5 0x7b537e54a81a in __assert_fail_base assert/assert.c:96:3
#6 0x7b537e55d516 in __assert_fail assert/assert.c:105:3
#7 0x57051c029cec in leveldb::BlockBuilder::Add(leveldb::Slice const&, leveldb::Slice const&) /table/block_builder.cc:74:3
#8 0x57051bfe9335 in leveldb::TableBuilder::Add(leveldb::Slice const&, leveldb::Slice const&) /table/table_builder.cc:117:17
#9 0x57051bfe6c01 in main /poc.cpp:28:7
#10 0x7b537e54c1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#11 0x7b537e54c28a in __libc_start_main csu/../csu/libc-start.c:360:3
#12 0x57051befb834 in _start (/poc+0x65834) (BuildId: 64073d6c603422592b135d549c9ef51f8ce13500)

Register values:
rax = 0x0000000000000000 rbx = 0x0000000000003ea0 rcx = 0x00007b537e5c0b2c rdx = 0x0000000000000006
rdi = 0x0000000000003ea0 rsi = 0x0000000000003ea0 rbp = 0x00007ffca2274480 rsp = 0x00007ffca2274440
r8 = 0x00000000000000bd r9 = 0x000079937d7e0000 r10 = 0x0000000000000008 r11 = 0x0000000000000246
r12 = 0x0000000000000006 r13 = 0x000057051c0565eb r14 = 0x0000000000000016 r15 = 0x000057051c0676c0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT nptl/pthread_kill.c:44:76 in __pthread_kill_implementation
```

## Reproduction Step
```bash
# 1. Checkout leveldb at the version above
git clone https://github.com/google/leveldb.git
cd leveldb
git checkout 7ee830d02b623e8ffe0b95d59a74db1e58da04c5

# 2. Build the library (sanitizer flags)
export CC=clang CXX=clang++
export CFLAGS="-fsanitize=address,undefined -g -O0 -fPIC -fno-omit-frame-pointer -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"
export CXXFLAGS="$CFLAGS"
export WORK=$(pwd)/build/sanitizer
export LEVELDB_SOURCE=$(pwd)
mkdir -p build && cd build
cmake -DCMAKE_INSTALL_PREFIX="$WORK" \
-DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" \
-DCMAKE_C_FLAGS="$CFLAGS" -DCMAKE_CXX_FLAGS="$CXXFLAGS" \
-DBUILD_SHARED_LIBS=OFF \
-DLEVELDB_BUILD_TESTS=OFF \
-DLEVELDB_BUILD_BENCHMARKS=OFF \
-DLEVELDB_INSTALL=ON \
..
make -j$(nproc) && make install

# 3. Build the PoC against that build
clang++ -g -O0 -fsanitize=address,undefined \
-I"$LEVELDB_SOURCE" \
-I"$WORK/include" \
poc.cpp -o poc \
-Wl,--start-group $WORK/lib/lib*.a -Wl,--end-group

# 4. Run
./poc
```

## Suggested Fix

The simplest correct fix is to flush the current data block before applying the new options, so `counter_` is reset via `BlockBuilder::Reset()`:

```cpp
// table/table_builder.cc — inside ChangeOptions, before rep_->options = options
if (!rep_->data_block.empty()) {
Flush();
}
rep_->options = options;
rep_->index_block_options = options;
rep_->index_block_options.block_restart_interval = 1;
```

This ensures the current block is written with consistent restart points before the new interval takes effect. An alternative is to weaken the assertion in `block_builder.cc:74` to `assert(counter_ >= 0)`, since the `else` branch already self-corrects — though flushing is preferable as it avoids writing a block with an irregular restart-point pattern.

## Submission Statement

This report was produced by FuzzAnything's AI-assisted library fuzzer and manually verified by a team member. We reviewed the PoC against the upstream API documentation — call order, parameters, and memory ownership — and found no API misuse.

> Signed-off-by: FuzzAnything

Contributor guide

Open the contributing guide

Research direction

Start with TableBuilder::ChangeOptions in table/table_builder.cc and BlockBuilder::Add in table/block_builder.cc, then run the supplied PoC against the stated revision. Trace how changing block_restart_interval affects the live data block and its counter_. Done means the assertion no longer aborts after a lower interval is applied, with regression coverage for the reproduced sequence.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.