Fix crash-causing pthread race condition & rework pthread implementation #47
Conversation
When the app tries to use a mutex function such as pthread_mutex_lock,
we run mutex_static_initializer if the mutex isn't initialized.
mutex_static_initializer internally calls pthread_mutex_init, which in
turn sets up the wrapped native mutex of the shim mutex struct.
A problem arises in the following scenario: thread 1 calls
pthread_mutex_init; thread 2 calls pthread_mutex_lock. In this case,
pthread_mutex_lock thinks the mutex isn't initialized, so it calls the
static initializer which in turn calls pthread_mutex_init, setting the
wrapped native mutex pointer. pthread_mutex_init runs at the same time
but on a different thread, messing up the internal state.
The solution is two-pronged:
1. use an atomic integer to determine whether a mutex is initialized;
2. protect pthread_mutex_init exposed to the app, as well as
pthread_static_initializer, with the same mutex internally, thus
preventing them from racing.
src/pthreads.h
Outdated
|
|
||
| namespace bionic { | ||
|
|
||
| // EXPECTED: Size: 40 bytes, alignment 8 bytes |
There was a problem hiding this comment.
don't we break 32bit abi? by removing the #ifdef ?
I would want to restore the 32bit abi before merging.
There was a problem hiding this comment.
I think I've addressed this now. I do not have a 32-bit setup to test on at the moment, but it should be correct.
src/pthreads.h
Outdated
| std::atomic_int64_t check = 0; | ||
| int64_t priv = 0; | ||
| #else | ||
| // EXPECTED: Size: 24 bytes, alignment 4 bytes |
There was a problem hiding this comment.
Somehow this tells me, we have 4bytes
https://android.googlesource.com/platform/bionic/+/master/libc/bionic/pthread_mutex.cpp#247
There was a problem hiding this comment.
Ah, I see now... You're definitely correct, yes. I'll need to store just an ID for the mutex like how Bionic does it, then have a mapping of ID to internal mutex info? I don't see how else I can get around the 4 byte size limitation.
There was a problem hiding this comment.
Newest changes should restore 32-bit compat. The 32-bit ABI does not benefit from the race condition fix, but it should work as it did before. I was able to test this time around by using the 32-bit pthread_mutex_t structure on a 64-bit build, which surprisingly did work, MCPE doesn't seem to rely on the structure's size.
Is there a reason why we're accurate to Bionic so deeply in the first place if apps aren't actually dependent on the ABI level size?
There was a problem hiding this comment.
having a smaller mutex is fine for me, but if our layout is larger than bionic we have write after free, altering following fields errors etc.
|
Verification failed for me: Not sure why. Get tons of these fatal errors on macOS. Atomics was always something that was too transparent to me, in terms what happens if you are not the one who initialize the value. |
|
I believe this is because OSX on ARM64 has different byte alignment..? I will try to force it to 8-byte alignment. |
Does not seem to change anything, alignment requirements cannot be enforced by the libc-shim compiler. The alignment is done by the side allocating the memory, in this case the android shared library. |
2 participants
This PR both reworks the pthread implementation in various areas, implements pthread functions more accurately where possible, such as
pthread_cond_timedwait_monotonic_np, and fixes the long standing pthread_mutex crashes.When the app tries to use a mutex function such as pthread_mutex_lock, we run mutex_static_initializer if the mutex isn't initialized. mutex_static_initializer internally calls pthread_mutex_init, which in turn sets up the wrapped native mutex of the shim mutex struct.
A problem arises in the following scenario: thread 1 calls pthread_mutex_init; thread 2 calls pthread_mutex_lock. In this case, pthread_mutex_lock thinks the mutex isn't initialized, so it calls the static initializer which in turn calls pthread_mutex_init, setting the wrapped native mutex pointer. pthread_mutex_init runs at the same time but on a different thread, messing up the internal state.
The solution is two-pronged: