danburkert / lmdb-rs

Safe Rust bindings for LMDB
Apache License 2.0
172 stars 87 forks source link

iter getter test that segfaults #32

Open mykmelez opened 6 years ago

mykmelez commented 6 years ago

This test demonstrates a segfault when returning an Iter from a function that also creates (and then drops) the Cursor with which the Iter is created. Presumably the issue is that dropping the Cursor calls mdb_cursor_close() on the MDB_cursor pointer, after which the Iter tries to use that pointer.

Cursor.cursor() says that "the caller must ensure that the pointer is not used after the lifetime of the cursor." But the Iter implementation doesn't enforce that constraint. And while direct consumers of Cursor.cursor() can't say they weren't warned, the same isn't true for indirect consumers who are calling the iter getters like Cursor.iter() (and potentially wrapping them in their own getter functions). Which makes them a footgun.

danburkert commented 6 years ago

Thanks for pointing this out, this test is exposing a very serious flaw in the current iterator API. The Iterator types should be taking ownership of the cursor. It gets a bit more complicated for the dup iterators, since they would then need to share the cursor among the dup iter and the sub-iterators, and as far as I know there's no way to tie the lifetime of the item of an iterator to the iterator itself. Might take a bit to find a solution to this.

tarcieri commented 6 years ago

and as far as I know there's no way to tie the lifetime of the item of an iterator to the iterator itself.

There’s StreamingIterator:

https://github.com/sfackler/streaming-iterator

danburkert commented 6 years ago

Yeah, that might be a good start. Technically we don't need the iterator items to actually be references, they just need to not be able to outlive the producing iterator. Having them be references / using streaming iterator may be the most straightforward way to do that.

In the short term I'm going to put together a patch to fix the Iter issue, since I think the fix is straightforward. We can fix IterDup is where it gets tricky.

tarcieri commented 6 years ago

Technically we don't need the iterator items to actually be references

Speaking of that...

https://lukaskalbertodt.github.io/2018/08/03/solving-the-generalized-streaming-iterator-problem-without-gats.html#not-the-solution-we-want-the-crate-streaming-iterator