Raw iterator segfaults when user forgets to seek before calling next

[athre0z]

Originally tested with v0.21.0 but also repros with master.

Add this test case to tests/test_raw_iterator.rs:

#[test]
pub fn test_forgot_seek() {
    let n = DBPath::new("test_forgot_seek");
    {
        let db = DB::open_default(&n).unwrap();
        db.put(b"k1", b"v1").unwrap();
        db.put(b"k2", b"v2").unwrap();
        db.put(b"k4", b"v4").unwrap();

        let mut iter = db.raw_iterator();
        iter.next();
    }
}

running 1 test
error: test failed, to rerun pass `--test test_raw_iterator`

Caused by:
  process didn't exit successfully: `/home/ath/devel/rust-rocksdb/target/debug/deps/test_raw_iterator-989d6e1a6868845b test_forgot_seek` (signal: 11, SIGSEGV: invalid memory reference)

[Congyuwang]

Atomic loading a null 0x0 address? Looks like a rocksdb bug. Does this occur if testing with c++ rocksdb of the same version?

[aleksuss]

@athre0z good catch 👍🏻

[aleksuss]

In cpp I've got the same result:

#include <rocksdb/db.h>

using namespace rocksdb;

int main() {
    DB *db = nullptr;
    Options options;

    options.create_if_missing = true;

    auto status = DB::Open(options, "/tmp/dddd", &db);
    assert(status.ok());

    auto iter = db->NewIterator(ReadOptions());
    iter->Next();


    return 0;
}

Result:

Current executable set to 'с++/cmake-build-debug/cpp-playground' (arm64).
(lldb) r
Process 9702 launched: 'с++/cmake-build-debug/cpp-playground' (arm64)
Process 9702 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001010246fc librocksdb.8.dylib`rocksdb::(anonymous namespace)::SkipListRep::Iterator::Next() + 4
librocksdb.8.dylib`rocksdb::(anonymous namespace)::SkipListRep::Iterator::Next:
->  0x1010246fc <+4>:  ldapr  x8, [x8]
    0x101024700 <+8>:  str    x8, [x0, #0x10]
    0x101024704 <+12>: ret    

librocksdb.8.dylib`rocksdb::(anonymous namespace)::SkipListRep::Iterator::~Iterator:
    0x101024708 <+0>:  stp    x20, x19, [sp, #-0x20]!
Target 0: (cpp-playground) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001010246fc librocksdb.8.dylib`rocksdb::(anonymous namespace)::SkipListRep::Iterator::Next() + 4
    frame #1: 0x0000000100f5b6d0 librocksdb.8.dylib`rocksdb::MemTableIterator::Next() + 64
    frame #2: 0x0000000100f5b718 librocksdb.8.dylib`rocksdb::MemTableIterator::NextAndGetResult(rocksdb::IterateResult*) + 36
    frame #3: 0x0000000100f1f034 librocksdb.8.dylib`rocksdb::DBIter::Next() + 308
    frame #4: 0x00000001000054b4 cpp-playground`main at main.cpp:27:11
    frame #5: 0x000000018cf7d058 dyld`start + 2224

[athre0z]

Hmm, yeah. The C++ Next method is annotated with a REQUIRES: Valid() comment. Valid() is indeed returning false here, so calling Next is not permitted:

https://github.com/facebook/rocksdb/blob/d7567d5eee5a0210376ce25475ae95b88b0a9c14/include/rocksdb/iterator.h#L67-L70

I suppose the problem here might simply be that in a C++ API it is typically considered to be perfectly acceptable for something to segfault if a user disregards such requirements whereas in Rust it would be considered unsound for a function that isn't marked as unsafe.

I suspect that the Rust interface for the raw iterator might have to be changed to automatically perform valid calls before next and prev. Alternatively the next and prev functions could be marked as unsafe. It looks like a similar check is already in place in key and value. Will probably eat a bit of CPU cycles for no good reason to also do it in next/prev, though not sure whether it'd be significant.

[Congyuwang]

Valid call seems very cheap, just returning a flag:

https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_iter.h#L144