bcoin-org / bdb Goto Github PK

There is a memory safety issue when making batch database calls.

Details:

Within a single process, the same leveldb::DB object may be safely shared by multiple concurrent threads. (...) However other objects (like Iterator and WriteBatch) may require external synchronization. If two threads share such an object, they must protect access to it using their own locking protocol. (...)

https://github.com/google/leveldb/blob/master/doc/index.md#concurrency

The bindings (./src/bindings.cc) methods batch_init, batch_put, batch_del, batch_write and batch_clear all share access to the WriteBatch object. The batch_write method will share the object with another thread in BatchWriteWorker. It's possible that batch_put, batch_del and batch_clear can unsafely modify the WriteBatch object that is shared with the other thread.

Reproduce:

Call batch_put, batch_del or batch_clear after a call to batch_write and before the batch_write worker thread is complete.

Some of the possible errors:

• Segmentation fault
• Abort trap 6
• Corruption: unknown WriteBatch tag
• Corruption: bad WriteBatch Put
• Corruption: WriteBatch has wrong count

I was experimenting with buffer maps as well, I also looked into bdb to get BufferMap not convert buffers at all (Similar to std::map) because of the js memory overhead when working with strings (fortunately I don't need huge keys), I ran several benchmarks today after seeing
bcoin-org/bcoin/issues/533

Is there any reason to use rbt for memdb ?

@chjj

Leveldown had support for string encoding for keys and values, but bdb depends and strictly uses buffers for everything. So we can drop support for strings and simplify the codebase. (no need for converting functions etc.)

DB Layout for key:

  e: bdb.key('e', ['uint32']),

Range Usage:

    return db.range({
      gte: layout.e.min(),
      lte: layout.e.max(),
      parse: key => layout.e.parse(key)
    });

If there's 0 written in that field, iterator.parse skips that value.
Issue comes from here: https://github.com/bcoin-org/bdb/blob/master/lib/db.js#L1162

We need some convention, what you need to return from parser to skip that value.

E.g. null or undefined.

It will probably skip zero string and other falsy values )))

@chjj

I can just return buffers and parse on my own. but still I think we need some convention when to skip in iterator.