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

Make the del/put_current and append Cursor methods unsafe #108

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

Kerollmops
Copy link
Member

@Kerollmops Kerollmops commented Jun 28, 2021

This PR emerges from a bug that we found in the milli repository, an undefined behavior due to a bad usage of LMDB. We were keeping borrowed data from an LMDB database while modifying it by using put_current at the same time, this is prohibited!

Values returned from the database are valid only until a subsequent update operation, or the end of the transaction.

In this PR we simply declare the del_current, put_current, and append iterator methods as unsafe and improved the documentation of them by introducing a Safety paragraph explaining the safe way to use these methods. The right way to use these methods is to just to_owned the key and value from the database before using this method.

@Kerollmops Kerollmops changed the title Make the del_current, put_current, and append Cursor methods unsafe Make the del/put_current, and append Cursor methods unsafe Jun 28, 2021
@Kerollmops Kerollmops changed the title Make the del/put_current, and append Cursor methods unsafe Make the del/put_current and append Cursor methods unsafe Jun 28, 2021
@Kerollmops Kerollmops merged commit aa9b340 into v0.12 Jun 28, 2021
@Kerollmops Kerollmops deleted the unsafe-put-del-current branch June 28, 2021 11:08
Kerollmops added a commit to meilisearch/milli that referenced this pull request Jun 28, 2021
It is undefined behavior to keep a reference to the database while
modifying it, we were keeping references in the database and also
feeding the heed put_current methods with keys referenced inside
the database itself.

meilisearch/heed#108
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.

2 participants