libc: make dl_phdr_iter lock recursive for LSan

[aokblast]

Summary

LSan may invoke dl_phdr_iter from different sanitizer contexts (e.g.,
ASan calling into LSan), which can result in recursive lock acquisition.
Use a recursive lock for dl_phdr_iter and initialize it through
pthread_once to ensure the lock is properly set up before use.

[kib]

You do understand, that the change is only changes something for the statically linked binaries?

[kevans]

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

We had already implemented a workaround in rtld, _dl_iterate_phdr_locked iirc. I'm not sure offhand why that wasn't sufficient here, I might need to re-check which lsan drop I sent to @aokblast

[aokblast]

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

Oh sorry. I see the RTLD_DEFAULT inside, it must calls the implementation from RTLD. I will check this more carefully. Thanks!

[aokblast]

In D58152#1333568, @kevans wrote:

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

We had already implemented a workaround in rtld, _dl_iterate_phdr_locked iirc. I'm not sure offhand why that wasn't sufficient here, I might need to re-check which lsan drop I sent to @aokblast

Yes, I see the comment from your brach and see both glibc and NetBSD (although very strange in their implemetation) has recursive dl_phdr_info_lock lock.

[kib]

In D58152#1333569, @aokblast wrote:

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

Oh sorry. I see the RTLD_DEFAULT inside, it must calls the implementation from RTLD. I will check this more carefully. Thanks!

The (manual) redirect to rtld is for the case when something used dlsym() to resolve dl_iterate_phdr() from libc. See 21a52f99440c9bec7

[kevans]

In D58152#1333577, @aokblast wrote:

In D58152#1333568, @kevans wrote:

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

We had already implemented a workaround in rtld, _dl_iterate_phdr_locked iirc. I'm not sure offhand why that wasn't sufficient here, I might need to re-check which lsan drop I sent to @aokblast

Yes, I see the comment from your brach and see both glibc and NetBSD (although very strange in their implemetation) has recursive dl_phdr_info_lock lock.

Yes, I think @kib was at least mildly concerned when I raised the thought of doing the same, and we decided it was reasonably fine to just bypass the locks because we do dl_iterate_phdr, rfork (vfork) inside the handler, then ptrace the parent. The thread that holds the relevanr locks is suspended in rfork and safely not racing against what we do in the child.

[aokblast]

In D58152#1333581, @kevans wrote:

In D58152#1333577, @aokblast wrote:

In D58152#1333568, @kevans wrote:

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

We had already implemented a workaround in rtld, _dl_iterate_phdr_locked iirc. I'm not sure offhand why that wasn't sufficient here, I might need to re-check which lsan drop I sent to @aokblast

Yes, I see the comment from your brach and see both glibc and NetBSD (although very strange in their implemetation) has recursive dl_phdr_info_lock lock.

Yes, I think @kib was at least mildly concerned when I raised the thought of doing the same, and we decided it was reasonably fine to just bypass the locks because we do dl_iterate_phdr, rfork (vfork) inside the handler, then ptrace the parent. The thread that holds the relevanr locks is suspended in rfork and safely not racing against what we do in the child.

Does something like:

_rtld_atfork_pre(rtld_locks);
StopTheWorld(callback, argument);
_rtld_atfork_post(rtld_locks);

make sense? What if a thread uses dlopen (hold bind_lock in callback context) in the callback context of dl_iterate_phdr (it releases bind_lock) and calls another LSan watcher thread? It makes the dl_open thread holds the bind_lock but lsan is requiring bind_lock.

[kevans]

In D58152#1333589, @aokblast wrote:

In D58152#1333581, @kevans wrote:

Yes, I think @kib was at least mildly concerned when I raised the thought of doing the same, and we decided it was reasonably fine to just bypass the locks because we do dl_iterate_phdr, rfork (vfork) inside the handler, then ptrace the parent. The thread that holds the relevanr locks is suspended in rfork and safely not racing against what we do in the child.

Correcting myself here: I thought we did, but I'm wondering if it deadlocked when I tried that because we needed to resolve some symbols in the lsan tracer. It's also rfork(RFPROC|RFMEM), not the vfork-ish flavor; the child does immediately enter wait(2) for the tracer to exit.

Does something like:

_rtld_atfork_pre(rtld_locks);
StopTheWorld(callback, argument);
_rtld_atfork_post(rtld_locks);

make sense? What if a thread uses dlopen (hold bind_lock in callback context) in the callback context of dl_iterate_phdr (it releases bind_lock) and calls another LSan watcher thread? It makes the dl_open thread holds the bind_lock but lsan is requiring bind_lock.

At a minimum, I does end up being a little more complicated because the child does share the address space, but maybe that's viable... deferring to kib.

[kib]

I think you need atfork_pre() in parent, not in the child.
There is indeed more places to avoid the bind lock in the child.