Exhaustive list of memory safety precautions

[braydonf]

There are several mentions about safety on the leveldown documentation:

From the README:

Although we are working to improve the safety of the leveldown interface it is still easy to crash your Node process if you don't do things in just the right way.

From the README Safety section:

(...) calling open() on an already open database may result in an error. Likewise, calling any other operation on a non-open database may result in an error.

I know of an additional concern:

For example the LevelDB documentation states:

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. (...)

I have noticed such a thread safety issue when calling batch_put, batch_del, batch_clear or db_close after batch_write. I have detailed this issue at bcoin-org/bdb#6, and have a reproducible test case for it at bcoin-org/bdb#7 (I can create a pull request to leveldown, if there is interest).

However, I am still seeing unexplained behavior with the below --debug backtrace:

Thread 10 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffde4a9700 (LWP 1816)]
0x00007ffff40d9a0e in leveldb::DecodeFixed64 (ptr=0x72f6fe000b8cb401 <error: Cannot access memory at address 0x72f6fe000b8cb401>) at ../deps/leveldb/leveldb-1.20/util/coding.h:76
76	    memcpy(&result, ptr, sizeof(result));  // gcc optimizes this to a plain load

(gdb) backtrace
#0  0x00007ffff40d9a0e in leveldb::DecodeFixed64 (ptr=0x72f6fe000b8cb401 <error: Cannot access memory at address 0x72f6fe000b8cb401>) at ../deps/leveldb/leveldb-1.20/util/coding.h:76
#1  0x00007ffff40fa1f2 in leveldb::WriteBatchInternal::Sequence (b=0x3cd47a0) at ../deps/leveldb/leveldb-1.20/db/write_batch.cc:91
#2  0x00007ffff40fa427 in leveldb::WriteBatchInternal::InsertInto (b=0x3cd47a0, memtable=0x7fffd0003150) at ../deps/leveldb/leveldb-1.20/db/write_batch.cc:131
#3  0x00007ffff40d7a59 in leveldb::DBImpl::Write (this=0x7fffd0000e20, options=..., my_batch=0x3cd47a0) at ../deps/leveldb/leveldb-1.20/db/db_impl.cc:1233
#4  0x00007ffff40cc4f7 in Database::WriteBatch (this=0x2c5fe80, options=..., batch=0x3cd47a0) at ../src/binding.cc:377
#5  0x00007ffff40cf28a in Batch::Write (this=0x42b17a0, sync=true) at ../src/binding.cc:1640
#6  0x00007ffff40cf3bf in BatchWriteWorker::DoExecute (this=0xb207970) at ../src/binding.cc:1730
#7  0x00007ffff40cbe4c in BaseWorker::Execute (env=0x2b506d0, data=0xb207970) at ../src/binding.cc:267
#8  0x00000000012ca1ae in worker (arg=0x0) at ../deps/uv/src/threadpool.c:122
#9  0x00007ffff6f146ba in start_thread (arg=0x7fffde4a9700) at pthread_create.c:333
#10 0x00007ffff6c4a41d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Are there any additional safety concerns that should be noted and may explain the above issue?

[vweevers]

What's bdb, and how does it relate to leveldown?

[vweevers]

At quick glance it looks like a fork. If so, from what version did y'all fork? Because we fixed several segfault situations this year.

[braydonf]

Okay, that's great :) As far as I know, bdb vendors and packages leveldown. I'll check to see which version is included.

[braydonf]

Comparing the binding.cc and this fix in particular isn't included yet #597, which could be related.

[braydonf]

I believe the included version was v5.0.0-0 from December 17th, 2018. I am working on updating to verify if issues are resolved with the fixes. Thanks!

[vweevers]

In addition to #597, there's #514, #612, #618, #621.

[braydonf]

Okay, so far so good. I think the GC related segfault fixes has resolved it.

[vweevers]

Good to close, then?

[braydonf]

Yeah, I think so, in regards to the mentioned stack.

However, there may still be a potential thread safety issue that should be noted or patched, see: bcoin-org/bdb@5abd296 (rebased on latest leveldown/binding.cc)

[vweevers]

I'm not sure if or how it's relevant to leveldown. If you can write a test that shows a bug in leveldown, that would be appreciated!

[braydonf]

It's indeed a potential segfault within leveldown/binding.cc. I do not see anything in leveldown/leveldown.js that would prevent such a case.

[vweevers]

We also have protections in place in abstract-leveldown, so it may not apply.

[vweevers]

The problem is doing any operations after batch.write(), correct? If so, we're guarded against that in abstract-leveldown.

[braydonf]

Yeah, was just looking at abstract-leveldown as well, and I don't see anything, nor in levelup.

[vweevers]

https://github.com/Level/abstract-leveldown/blob/1c370da94ab9b23a6c8ad03349af3f6b6bb61307/abstract-chained-batch.js#L11-L15

[braydonf]

Okay, I see. So it's a question if it should be part of binding.cc, as similar to the check added in #618. And as similar if the iterator check from #514 should also be part of the binding.

[vweevers]

Not at this time, as 99% of consumers don't use the native addon directly, and we clearly document the recommended way to consume leveldown. The added complexity would not be worth it IMO.

[braydonf]

Okay. However, I do think thread safety of BatchWrite is a C++ concern and not a JS one, and should long-term be a part of binding.cc.

[vweevers]

Long-term, maybe. The (JS) protections of abstract-leveldown also cover other implementations like rocksdb, so that's a more convenient place now, while we're working on other things.