Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iter getter test that segfaults #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mykmelez
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

tarcieri commented Aug 3, 2018

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 danburkert mentioned this pull request Aug 3, 2018
@danburkert
Copy link
Owner

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
Copy link
Contributor

tarcieri commented Aug 4, 2018

ncloudioj pushed a commit to ncloudioj/lmdb-rs that referenced this pull request Mar 13, 2019
….8.2

update patch version for lmdb-rkv-sys 0.8.2
@victorporof victorporof deleted the iter-getter-segfaults branch August 30, 2019 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants