LD_PRELOAD with .init_array: fatal runtime error: thread::set_current should only be called once per thread

CAD97 · September 11, 2024, 2:48pm

Use ThreadId instead of TLS-address in `ReentrantLock`

rust-lang:master ← Sp00ph:reentrant_lock_tid

opened 08:48AM - 08 May 24 UTC

Fixes #123458 `ReentrantLock` currently uses the address of a thread local va…riable as an ID that's unique across all currently running threads. This can lead to uninituitive behavior as in #123458 if TLS blocks get reused. This PR changes `ReentrantLock` to instead use the `ThreadId` provided by `std` as the unique ID. `ThreadId` guarantees uniqueness across the lifetime of the whole process, so we don't need to worry about reusing IDs of terminated threads. The main appeal of this PR is thus the possibility of changing the `ReentrantLock` API to guarantee that if a thread leaks a lock guard, no other thread may ever acquire that lock again. This does entail some complications: - previously, the only way to retrieve the current thread ID would've been using `thread::current().id()` which creates a temporary `Arc` and which isn't available in TLS destructors. As part of this PR, the thread ID instead gets cached in its own thread local, as suggested [here](https://github.com/rust-lang/rust/issues/123458#issuecomment-2038207704). - `ThreadId` is always 64-bit whereas the current implementation uses a usize-sized ID. Since this ID needs to be updated atomically, we can't simply use a single atomic variable on 32 bit platforms. Instead, we fall back to using a (sound) seqlock on 32-bit platforms, which works because only one thread at a time can write to the ID. This seqlock is technically susceptible to the ABA problem, but the attack vector to create actual unsoundness has to be very specific: - You would need to be able to lock+unlock the lock exactly 2^31 times (or a multiple thereof) while a thread trying to lock it sleeps - The sleeping thread would have to suspend after reading one half of the thread id but before reading the other half - The teared result from combining the halves of the thread ID would have to exactly line up with the sleeping thread's ID The risk of this occurring seems slim enough to be acceptable to me, but correct me if I'm wrong. This also means that the size of the lock increases by 8 bytes on 32-bit platforms, but this also shouldn't be an issue. Performance wise, I did some crude testing of the only case where this could lead to real slowdowns, which is the case of locking a `ReentrantLock` that's already locked by the current thread. On both aarch64 and x86-64, there is (expectedly) pretty much no performance hit. I didn't have any 32-bit platforms to test the seqlock performance on, so I did the next best thing and just forced the 64-bit platforms to use the seqlock implementation. There, the performance degraded by ~1-2ns/(lock+unlock) on x86-64 and ~6-8ns/(lock+unlock) on aarch64, which is measurable but seems acceptable to me seeing as 32-bit platforms should be a small minority anyways. cc @joboet @RalfJung @CAD97

I suppose something in the std IO machinery is using ReentrantLock, if this is actually the correct cause? Although that's purely just a guess.

Topic		Replies	Views
Cargo.toml to include LD_PRELOAD in cargo run?	5	840	October 6, 2019
Cannot pass LD_PRELOAD with cargo	4	785	June 10, 2021
LD_PRELOAD init function in Rust? help	7	1784	January 12, 2023
LD_PRELOAD works as dylib and doesn't as cdylib. Which is the right choice? help	15	1283	December 2, 2020
Shared library constructor code, seems to trigger brk/mmap which then seems to trigger reloading the shared library again help	1	407	February 2, 2021

LD_PRELOAD with .init_array: fatal runtime error: thread::set_current should only be called once per thread

Related topics