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

Fix two memory safety issues. #1

Merged
merged 2 commits into from
Sep 14, 2016
Merged

Fix two memory safety issues. #1

merged 2 commits into from
Sep 14, 2016

Conversation

cstorey
Copy link
Contributor

@cstorey cstorey commented Sep 14, 2016

I've got an application which happens to re-size the allocate lmdb mapping by closing and re-opening the database, as that happens to be easier than ensuring the DB is quiescent first in this case.

I ended up finding that my program would segmentation fault when beginning transactions, or whilst the runtime was cleaning up an exited thread. Using valgrind, I tracked down two issues:

in Database::open, we allocate an Option<CString> to hold the database name. However, Option::map consumes it's value, so any contained CString is dropped after the call to CString::as_ptr, but before we pass it to mdb_dbi_open. So we have an effective use-after-free.

I'd found that a MDB_txn was getting freed twice inside of mdb_txn_abort, and then in mdb_env_close. It turns out that an earlier call to mdb_txn_comit was failing, so the transaction had been marked as finished, but the use of the lmdb_call! macro meant we didn't zero out the TxHandle content. So, we ended up calling mdb_txn_abort on an already finalized handle. Then, because of the state of the transaction flags, we ended up freeing the memory that was referenced by the environment, as it's me_txn0 member.

Anyway, I hope this helps, and thanks for the library!

@AltSysrq
Copy link
Owner

Thanks for the PR @cstorey! I actually thought mdb_txn_commit() left the transaction open if it fails, but as you hinted it does in fact call mdb_txn_abort() implicitly.

This looks good on all points, merging.

@AltSysrq AltSysrq merged commit e053345 into AltSysrq:master Sep 14, 2016
@AltSysrq
Copy link
Owner

Uploaded 0.2.1 with this fix.

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