[musl] Re: [PATCH v9 0/13] implement dlmem() function (fwd)

[musl] Re: [PATCH v9 0/13] implement dlmem() function (fwd)

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

Well this is disturbing. We probably need to fix gcc here (and a lot
of code in the wild may be broken) because musl has no such locking
where it doesn't belong.

[-- Attachment #2: Type: message/rfc822, Size: 3384 bytes --]

From: Florian Weimer <fweimer@redhat.com>
To: Szabolcs Nagy via Libc-alpha <libc-alpha@sourceware.org>
Cc: stsp <stsp2@yandex.ru>,  Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,  janderson@rice.edu,  Carlos O'Donell <carlos@redhat.com>,  Rich Felker <dalias@libc.org>,  Szabolcs Nagy <szabolcs.nagy@arm.com>
Subject: Re: [PATCH v9 0/13] implement dlmem() function
Date: Wed, 05 Apr 2023 11:31:53 +0200
Message-ID: <87fs9en08m.fsf@oldenburg.str.redhat.com>

* Szabolcs Nagy via Libc-alpha:

> The 04/05/2023 12:29, stsp wrote:
>> - dl_iterate_phdr() seems to be calling the user
>>   callback under dl_load_write_lock lock.
>
> this is a known bug.

It's also not something we can fix because the libgcc unwinder has code
on it that relies on this implicit loader lock to protect its internal
data structures.  The libgcc unwinder can be statically linked, so we
can't remove the locking without adding a new symbol version.

I suspect other uses of dl_iterate_phdr are similar.

Thanks,
Florian

[musl] Re: [PATCH v9 0/13] implement dlmem() function (fwd)

[Florian Weimer]

* Rich Felker:

> Well this is disturbing. We probably need to fix gcc here (and a lot
> of code in the wild may be broken) because musl has no such locking
> where it doesn't belong.

We have a helper function in glibc (_dl_find_object) that replaces all
the previous object traversal logic and caching in libgcc, so it's no
longer an issue with current gcc and glibc.  Unfortunately that means
that the generic dl_iterate_phdr code will be fixed by adding its own
locking.

Thanks,
Florian

Re: [musl] Re: [PATCH v9 0/13] implement dlmem() function (fwd)

[Rich Felker]

On Wed, Apr 05, 2023 at 02:13:16PM +0200, Florian Weimer wrote:
> * Rich Felker:
> 
> > Well this is disturbing. We probably need to fix gcc here (and a lot
> > of code in the wild may be broken) because musl has no such locking
> > where it doesn't belong.
> 
> We have a helper function in glibc (_dl_find_object) that replaces all
> the previous object traversal logic and caching in libgcc, so it's no
> longer an issue with current gcc and glibc.

Does gcc automatically detect and use this if available? What gcc
versions does that cover? We could certainly add it, but I suspect
coverage will be incomplete... :/

> Unfortunately that means
> that the generic dl_iterate_phdr code will be fixed by adding its own
> locking.

Not sure why that's unfortunate; it's what it should have been doing
all along.

Rich

Re: [musl] Re: [PATCH v9 0/13] implement dlmem() function (fwd)

[Florian Weimer]

* Rich Felker:

> On Wed, Apr 05, 2023 at 02:13:16PM +0200, Florian Weimer wrote:
>> * Rich Felker:
>> 
>> > Well this is disturbing. We probably need to fix gcc here (and a lot
>> > of code in the wild may be broken) because musl has no such locking
>> > where it doesn't belong.
>> 
>> We have a helper function in glibc (_dl_find_object) that replaces all
>> the previous object traversal logic and caching in libgcc, so it's no
>> longer an issue with current gcc and glibc.
>
> Does gcc automatically detect and use this if available? What gcc
> versions does that cover? We could certainly add it, but I suspect
> coverage will be incomplete... :/

It's using a macro from <dlfcn.h> as a proxy to indicate libc support:

+  /* Use DLFO_STRUCT_HAS_EH_DBASE as a proxy for the existence of a glibc-style
+     _dl_find_object function.  */
+#ifdef DLFO_STRUCT_HAS_EH_DBASE

It's not dependent upon __GLIBC__.

>> Unfortunately that means that the generic dl_iterate_phdr code will
>> be fixed by adding its own locking.
>
> Not sure why that's unfortunate; it's what it should have been doing
> all along.

I missed a negation.  I meant that It's unlikely that it's going to be
fixed, unless you write a patch yourself. 8->

Thanks,
Florian