Skip to content

Fix crash-causing pthread race condition & rework pthread implementation #47

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix crash-causing pthread race condition & rework pthread implementation #47

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb032b9
Fix crash-causing pthread race condition & rework pthread implementation
V3L0C1T13S Nov 6, 2025
f1505e0
fix pthread_mutex_t 32-bit ABI compat
V3L0C1T13S Nov 7, 2025
ed09a0d
restore 32-bit ABI compat
V3L0C1T13S Nov 7, 2025
499996a
fix: force pthread_mutex_t to have proper alignment regardless of pla…
V3L0C1T13S Nov 7, 2025
214f3c0
(hopefully) fix pthread_mutex_t on OSX
V3L0C1T13S Nov 15, 2025
f067f26
more accurately handle use of destroyed mutex
V3L0C1T13S Nov 15, 2025
b3771ab
add guard for nullptr mutex on 32-bit ABI
V3L0C1T13S Nov 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/pthreads.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,21 @@ namespace shim {

namespace bionic {

// EXPECTED: Size: 40 bytes, alignment 8 bytes
struct pthread_mutex_t {
#if defined(__LP64__)
// EXPECTED: Size: 40 bytes, alignment 8 bytes
size_t init_value = 0;
::pthread_mutex_t *wrapped = nullptr;
std::atomic_int64_t is_initialized = 0;
std::atomic_int64_t check = 0;
int64_t priv = 0;
#else
// EXPECTED: Size: 24 bytes, alignment 4 bytes
Copy link
Member

@ChristopherHX ChristopherHX Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow this tells me, we have 4bytes

https://android.googlesource.com/platform/bionic/+/master/libc/bionic/pthread_mutex.cpp#247

Copy link
Author

@V3L0C1T13S V3L0C1T13S Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@V3L0C1T13S V3L0C1T13S Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ChristopherHX ChristopherHX Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@V3L0C1T13S V3L0C1T13S Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, then.

size_t init_value = 0;
::pthread_mutex_t *wrapped = nullptr;
std::atomic_int32_t is_initialized = 0;
std::atomic_int32_t check = 0;
#endif
};

constexpr size_t mutex_init_value = 0;
Expand Down