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

Implement Send for RoTxn #191

Closed
MarinPostma opened this issue Jul 25, 2023 · 3 comments · Fixed by #192
Closed

Implement Send for RoTxn #191

MarinPostma opened this issue Jul 25, 2023 · 3 comments · Fixed by #192
Labels
breaking A change that is breaking the semver bug Something isn't working security Something that is security related
Milestone

Comments

@MarinPostma
Copy link
Contributor

MarinPostma commented Jul 25, 2023

I wanted to make the write transactions Send but LMDB's documentation says:

A write Transaction may only be used from the thread it was created on.

However, I think that making RoTxn: Sync with the sync-read-txn is not safe, but I may be mistaken. But, for sure, RoTxn should implement Send, since it can be moved to other threads safely.

@MarinPostma MarinPostma mentioned this issue Jul 25, 2023
3 tasks
@Kerollmops
Copy link
Member

Kerollmops commented Jul 25, 2023

According to the documentation it seems that you are right we can't refer to the read transaction from different threads, at least we must synchronize the use (by using a mutex?). According to Howard Chu, we must remove this sync-read-txn feature and introduce the new send-read-txn feature you want to implement instead.

What's cool is that if we implement RoTxn: Send, users will be able to use a Mutex to use it through threads as it only requires T: Send for the Mutex<T>: Sync.

MDB_NOTLS

Don't use Thread-Local Storage. Tie reader locktable slots to #MDB_txn objects instead of to threads. I.e. #mdb_txn_reset() keeps the slot reserved for the #MDB_txn object. A thread may use parallel read-only transactions. A read-only transaction may span threads if the user synchronizes its use. Applications that multiplex many user threads over individual OS threads need this option. Such an application must also serialize the write transactions in an OS thread, since LMDB's write locking is unaware of the user threads.

Yanking or Reporting a Security Issue?

I have some questions for the crates.io team or Rust developers in general, but should I yank all previous versions of the heed crate? Or should I raise a RustSec advisory about heed that concerns this specific sync-read-txn feature? The latter seems more appropriate as the library is safe to use until you use this particular feature.

Thank you for the investigation and report again 🙏

@Kerollmops Kerollmops added bug Something isn't working breaking A change that is breaking the semver labels Jul 25, 2023
@Kerollmops Kerollmops added this to the v0.20.0 milestone Jul 25, 2023
@Kerollmops Kerollmops added the security Something that is security related label Jul 26, 2023
meili-bors bot added a commit to meilisearch/meilisearch that referenced this issue Jul 26, 2023
3952: No more use the broken `sync-read-txn` heed feature r=curquiza a=Kerollmops

[We recently found out](meilisearch/heed#191 (comment)) that the `read-sync-txn` heed feature was invalid and must be removed from this crate. We were declaring it in milli/meilisearch but, fortunately, not sharing the `RoTxn`s across threads 😮‍💨

This PR removes the `sync-read-txn` heed feature form the _Cargo.toml_ file. I will fix this in heed v0.20.0 and will fill a RustSec advisory in the meantime.

Co-authored-by: Clément Renault <clement@meilisearch.com>
@Kerollmops
Copy link
Member

Reopening as I need to fill a RustSec advisory.

@Kerollmops Kerollmops reopened this Jul 26, 2023
meili-bors bot added a commit to meilisearch/meilisearch that referenced this issue Jul 26, 2023
3952: Use the new safe `read-txn-no-tls` heed feature r=ManyTheFish a=Kerollmops

[We recently found out](meilisearch/heed#191 (comment)) that the `read-sync-txn` heed feature was invalid and must be removed from this crate. We were declaring it in milli/meilisearch but, fortunately, not sharing the `RoTxn`s across threads 😮‍💨

[I recently introduced the `read-txn-no-tls` heed feature](meilisearch/heed#194), which implements `RoTxn: Send` and allows multiple read transactions on a single thread (which we use).

This PR removes the `sync-read-txn` heed feature from the _Cargo.toml_ file. I will fix this in heed v0.20.0 and will fill a RustSec advisory in the meantime.

Co-authored-by: Clément Renault <clement@meilisearch.com>
@Kerollmops
Copy link
Member

I just asked for help in rustsec/advisory-db#1755, and we will continue the discussion there, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that is breaking the semver bug Something isn't working security Something that is security related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants