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 _pthread_tid_offset check for older versions of glibc #95

Closed
wants to merge 1 commit into from

Conversation

iynehz
Copy link

@iynehz iynehz commented Oct 28, 2021

Requirements for Adding, Changing, Fixing or Removing a Feature

This fixes #93

For recent versions of glic, the pthread struct is like,

struct pthread {
    ...
    pid_t tid,          // size_of(pid_t) == 4
    pid_t pid_unused,   // value is 0
    ...
}

Before this commit, the _pthread_tid_offset check logic iterates the memory
block in every 8 bytes (uintptr_t), and it works because pid_unused is 0.

However, in older versions of glibc like 2.17 which is used by RHEL/Centos 7,
its pthread struct is like below and comparing 8 bytes it does not match
the tid because the pid field is non-zero.

struct pthread {
    ...
    pid_t tid,          // sizeof(pid_t) == 4
    pid_t pid,          // value is not 0 but pid
    ...
}

Description of the Change

Instead of iterating the memory block in uintptr_t size that is 8 bytes on 64bit Linux, iterate in pid_t size that should be 4 bytes.

Verification Process

Tested on both a Debian 11 host and a RHEL7 host.

For recent versions of glic, the pthread struct is like,

{
    ...
    pid_t tid,          // size_of(pid_t) == 4
    pid_t pid_unused,   // value is 0
    ...
}

Before this commit, the _pthread_tid_offset check logic iterates the memory
block in every 8 bytes (uintptr_t), and it works because pid_unused is 0.

However, in older versions of glibc like 2.17 which is used by RHEL/Centos 7,
its pthread struct is like below and comparing 8 bytes it does not match
the tid because the pid field is non-zero.

{
    ...
    pid_t tid,          // sizeof(pid_t) == 4
    pid_t pid,          // value is not 0 but pid
    ...
}
Copy link
Owner

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

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

Ah! So this is due to memory alignment. Thanks for debugging this, good catch! 👍 Just a comment, otherwise LGTM!

log_d("pthread_t at %p", py_thread->tid);
if (py_thread->raddr.pid == (uintptr_t) _pthread_buffer[i]) {
if (py_thread->raddr.pid == *((pid_t *) _pthread_buffer + i)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer if we did this as a fallback in case steps of size uniptr_t are failing. My concern is that this might end up accidentally matching something else. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I think if we use that as a fallback, that does not really protect you from accidentally matching something else, as it if the comparing uintptr_t fails, it would fallback to the comparing pid_t logic..

Do you think if it good we parse the output of gnu_get_libc_version(), as we can surely know from which glibc version it started to replace pid with pid_unused. So for glibc versions above that we compare uintptr_t, for lower glibc versions we compare pid_t.

Copy link
Owner

Choose a reason for hiding this comment

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

Do you think if it good we parse the output of gnu_get_libc_version()

This sounds good to me. Can we make it so that it is also compatible with musl as I'd like to start making these builds available too from releases? We should be able to import features.h with both glibc and musl, and check for __GLIBC__ and __GLIBC_MINOR__ (which en passant might save us from having to parse the string returned by gnu_get_libc_version?).

Copy link
Author

Choose a reason for hiding this comment

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

hmm I think the macro resolves to a fixed version at compile time, so the built binary is not portable. but the function could work at runtime. I will need to furthur check though.

Copy link
Owner

Choose a reason for hiding this comment

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

hmm I think the macro resolves to a fixed version at compile time

Ah, of course! We're not linking libc statically so indeed we'd have to call the function. But at least we should be able to use the macros to determine what we're compiling against 🤞.

P403n1x87 added a commit that referenced this pull request Dec 14, 2021
Older versions of glibc have a different struct pthread definition
that requires scanning the pthread buffer with smaller steps in
order to find the actual offset.

Mitigates #93. A cleaner fix will be provided by #95."
P403n1x87 added a commit that referenced this pull request Dec 14, 2021
Older versions of glibc have a different struct pthread definition
that requires scanning the pthread buffer with smaller steps in
order to find the actual offset.

Mitigates #93. A cleaner fix will be provided by #95.
P403n1x87 added a commit that referenced this pull request Dec 14, 2021
Older versions of glibc have a different struct pthread definition
that requires scanning the pthread buffer with smaller steps in
order to find the actual offset.

Mitigates #93. A cleaner fix will be provided by #95.
P403n1x87 added a commit that referenced this pull request Dec 14, 2021
Older versions of glibc have a different struct pthread definition
that requires scanning the pthread buffer with smaller steps in
order to find the actual offset.

Mitigates #93. A cleaner fix will be provided by #95.
P403n1x87 added a commit that referenced this pull request Dec 14, 2021
Older versions of glibc have a different struct pthread definition
that requires scanning the pthread buffer with smaller steps in
order to find the actual offset.

Mitigates #93. A cleaner fix will be provided by #95.
@P403n1x87 P403n1x87 added the stale Inactive for a long time label Sep 14, 2022
@P403n1x87
Copy link
Owner

Closing this for now. Should this still be a problem we can have another look at it.

@P403n1x87 P403n1x87 closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Inactive for a long time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fail to infer tid offset
2 participants