mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
@ 2025-09-30 21:23 Mike Hilgendorf
  2025-10-01  7:44 ` Alexander Monakov
  2025-10-01 12:06 ` Rich Felker
  0 siblings, 2 replies; 7+ messages in thread
From: Mike Hilgendorf @ 2025-09-30 21:23 UTC (permalink / raw)
  To: musl

I've failed to reproduce this bug except in rare cases, this is the
smallest I could make it.

Say you want to set LD_PRELOAD to run some code before main() and
unset LD_PRELOAD within that function so it only runs for the parent
process. You might do something like this:


```
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

__attribute__((constructor))
static int preload () {
    if (unsetenv("LD_PRELOAD")) {
        printf("unsetenv errored: %d\n", errno);
    }
    if (getenv("LD_PRELOAD")) {
        printf("LD_PRELOAD was still set\n");
    }
}
```
compiled by running:

musl-gcc preload.c -shared -o preload.so

Now you want to inject this into a binary, say bash, compiled against musl libc

```
cd bash-5.2.37
export CC=musl-gcc
./configure
make

env -i LD_PRELOAD=path/to/preload.so ./bash
```

You will see that LD_PRELOAD was not unset (even in the context of the
ctor function), and LD_PRELOAD is set in the shell.

From what I can tell, LD_PRELOAD is not special, this is true of other
environment variables. However this is not reproducible with simpler
programs - bash 5.2 is the one where I saw this happen first, and so
far the only program I know that has this issue.

Let me know if I'm doing something very wrong or if there is an easier
reproduction, from scanning bash's source I don't think they're doing
anything strange (they initialize variables with the `char** envp`
passed to main).

I do notice in `dynlink.c` that the stack pointer passed to the
entrypoint by the loader is whatever was passed to the loader, not
accounting for any envp mutations made by calling the DT_INIT_ARRAY
functions. But I don't know if this is actually a problem or not for
the implementation of _start.

- Mike Hilgendorf

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-09-30 21:23 [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function Mike Hilgendorf
@ 2025-10-01  7:44 ` Alexander Monakov
  2025-10-01 12:06 ` Rich Felker
  1 sibling, 0 replies; 7+ messages in thread
From: Alexander Monakov @ 2025-10-01  7:44 UTC (permalink / raw)
  To: musl


On Tue, 30 Sep 2025, Mike Hilgendorf wrote:

> cd bash-5.2.37
> export CC=musl-gcc
> ./configure
> make
> 
> env -i LD_PRELOAD=path/to/preload.so ./bash

Try

nm -D ./bash | grep env

If you see 'T' symbols, bash exports its own env functions, and your LD_PRELOAD
module invokes the implementations from bash, not libc.

Alexander

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-09-30 21:23 [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function Mike Hilgendorf
  2025-10-01  7:44 ` Alexander Monakov
@ 2025-10-01 12:06 ` Rich Felker
  2025-10-01 12:08   ` Rich Felker
  2025-10-01 13:49   ` Mike Hilgendorf
  1 sibling, 2 replies; 7+ messages in thread
From: Rich Felker @ 2025-10-01 12:06 UTC (permalink / raw)
  To: Mike Hilgendorf; +Cc: musl

On Tue, Sep 30, 2025 at 04:23:44PM -0500, Mike Hilgendorf wrote:
> I've failed to reproduce this bug except in rare cases, this is the
> smallest I could make it.
> 
> Say you want to set LD_PRELOAD to run some code before main() and
> unset LD_PRELOAD within that function so it only runs for the parent
> process. You might do something like this:
> 
> 
> ```
> #include <stdio.h>
> #include <stdlib.h>
> #include <errno.h>
> 
> __attribute__((constructor))
> static int preload () {
>     if (unsetenv("LD_PRELOAD")) {
>         printf("unsetenv errored: %d\n", errno);
>     }
>     if (getenv("LD_PRELOAD")) {
>         printf("LD_PRELOAD was still set\n");
>     }
> }
> ```
> compiled by running:
> 
> musl-gcc preload.c -shared -o preload.so
> 
> Now you want to inject this into a binary, say bash, compiled against musl libc
> 
> ```
> cd bash-5.2.37
> export CC=musl-gcc
> ./configure
> make
> 
> env -i LD_PRELOAD=path/to/preload.so ./bash
> ```
> 
> You will see that LD_PRELOAD was not unset (even in the context of the
> ctor function), and LD_PRELOAD is set in the shell.
> 
> From what I can tell, LD_PRELOAD is not special, this is true of other
> environment variables. However this is not reproducible with simpler
> programs - bash 5.2 is the one where I saw this happen first, and so
> far the only program I know that has this issue.

Are you sure it's reproducible with other programs? I suspect what's
happening is that bash is using the optional third argument envp to
main, not the actual current environment, as its source to derive the
initial environment list.

I don't see any way the issue you're describing could happen
otherwise.

> Let me know if I'm doing something very wrong or if there is an easier
> reproduction, from scanning bash's source I don't think they're doing
> anything strange (they initialize variables with the `char** envp`
> passed to main).

Yes, so it's exactly what I suspected.

> I do notice in `dynlink.c` that the stack pointer passed to the
> entrypoint by the loader is whatever was passed to the loader, not
> accounting for any envp mutations made by calling the DT_INIT_ARRAY
> functions. But I don't know if this is actually a problem or not for
> the implementation of _start.

No application code, not even DT_INIT_ARRAY handlers, has executed at
this point. It all runs after execution is passed to the main
program's ELF entry point (_start).

The behavior you're seeing has nothing to do with the dynamic linker.
It's line 95 of src/env/__libc_start_main.c passing a pointer to the
initial environment vector rather than any potentially-updated value
of environ. I'm not sure if this should be changed or not (there are
probably arguments for either), but bash using the nonstandard
third-arg to main rather than the standard global environ[] strikes me
as a bash bug.

Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-10-01 12:06 ` Rich Felker
@ 2025-10-01 12:08   ` Rich Felker
  2025-11-19 16:50     ` Markus Wichmann
  2025-10-01 13:49   ` Mike Hilgendorf
  1 sibling, 1 reply; 7+ messages in thread
From: Rich Felker @ 2025-10-01 12:08 UTC (permalink / raw)
  To: Mike Hilgendorf; +Cc: musl

On Wed, Oct 01, 2025 at 08:06:16AM -0400, Rich Felker wrote:
> On Tue, Sep 30, 2025 at 04:23:44PM -0500, Mike Hilgendorf wrote:
> > I've failed to reproduce this bug except in rare cases, this is the
> > smallest I could make it.
> > 
> > Say you want to set LD_PRELOAD to run some code before main() and
> > unset LD_PRELOAD within that function so it only runs for the parent
> > process. You might do something like this:
> > 
> > 
> > ```
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <errno.h>
> > 
> > __attribute__((constructor))
> > static int preload () {
> >     if (unsetenv("LD_PRELOAD")) {
> >         printf("unsetenv errored: %d\n", errno);
> >     }
> >     if (getenv("LD_PRELOAD")) {
> >         printf("LD_PRELOAD was still set\n");
> >     }
> > }
> > ```
> > compiled by running:
> > 
> > musl-gcc preload.c -shared -o preload.so
> > 
> > Now you want to inject this into a binary, say bash, compiled against musl libc
> > 
> > ```
> > cd bash-5.2.37
> > export CC=musl-gcc
> > ./configure
> > make
> > 
> > env -i LD_PRELOAD=path/to/preload.so ./bash
> > ```
> > 
> > You will see that LD_PRELOAD was not unset (even in the context of the
> > ctor function), and LD_PRELOAD is set in the shell.
> > 
> > From what I can tell, LD_PRELOAD is not special, this is true of other
> > environment variables. However this is not reproducible with simpler
> > programs - bash 5.2 is the one where I saw this happen first, and so
> > far the only program I know that has this issue.
> 
> Are you sure it's reproducible with other programs? I suspect what's
> happening is that bash is using the optional third argument envp to
> main, not the actual current environment, as its source to derive the
> initial environment list.
> 
> I don't see any way the issue you're describing could happen
> otherwise.
> 
> > Let me know if I'm doing something very wrong or if there is an easier
> > reproduction, from scanning bash's source I don't think they're doing
> > anything strange (they initialize variables with the `char** envp`
> > passed to main).
> 
> Yes, so it's exactly what I suspected.
> 
> > I do notice in `dynlink.c` that the stack pointer passed to the
> > entrypoint by the loader is whatever was passed to the loader, not
> > accounting for any envp mutations made by calling the DT_INIT_ARRAY
> > functions. But I don't know if this is actually a problem or not for
> > the implementation of _start.
> 
> No application code, not even DT_INIT_ARRAY handlers, has executed at
> this point. It all runs after execution is passed to the main
> program's ELF entry point (_start).
> 
> The behavior you're seeing has nothing to do with the dynamic linker.
> It's line 95 of src/env/__libc_start_main.c passing a pointer to the
> initial environment vector rather than any potentially-updated value
> of environ. I'm not sure if this should be changed or not (there are
> probably arguments for either), but bash using the nonstandard
> third-arg to main rather than the standard global environ[] strikes me
> as a bash bug.

Slight addendum: this is a big reason *why* folks should be using the
standard interfaces like environ[] rather than historical hacks like
the third arg envp: when there is a question about weird corner-case
behavior like we have here, there's an actual specification governing
what the behavior should be and usually a clear objective answer (and
if not, a consensus-based process for deciding if there should be one)
rather than "whatever your system happens to do".

Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-10-01 12:06 ` Rich Felker
  2025-10-01 12:08   ` Rich Felker
@ 2025-10-01 13:49   ` Mike Hilgendorf
  1 sibling, 0 replies; 7+ messages in thread
From: Mike Hilgendorf @ 2025-10-01 13:49 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

Confirmed, this was an issue in the way bash handles
setenv/unsetenv/getenv and environ. Apologies for the report.

The specific problem was that bash overrides the ***env functions
internally, so any kind of injection that relied on them agreeing on
those functions naturally wouldn't work.

There's no change to __libc_start_main required. I wasn't able to
observe the issue even using the third arg to main().

Sorry for the erroneous report!

- Mike

On Wed, Oct 1, 2025 at 7:06 AM Rich Felker <dalias@libc.org> wrote:
>
> On Tue, Sep 30, 2025 at 04:23:44PM -0500, Mike Hilgendorf wrote:
> > I've failed to reproduce this bug except in rare cases, this is the
> > smallest I could make it.
> >
> > Say you want to set LD_PRELOAD to run some code before main() and
> > unset LD_PRELOAD within that function so it only runs for the parent
> > process. You might do something like this:
> >
> >
> > ```
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <errno.h>
> >
> > __attribute__((constructor))
> > static int preload () {
> >     if (unsetenv("LD_PRELOAD")) {
> >         printf("unsetenv errored: %d\n", errno);
> >     }
> >     if (getenv("LD_PRELOAD")) {
> >         printf("LD_PRELOAD was still set\n");
> >     }
> > }
> > ```
> > compiled by running:
> >
> > musl-gcc preload.c -shared -o preload.so
> >
> > Now you want to inject this into a binary, say bash, compiled against musl libc
> >
> > ```
> > cd bash-5.2.37
> > export CC=musl-gcc
> > ./configure
> > make
> >
> > env -i LD_PRELOAD=path/to/preload.so ./bash
> > ```
> >
> > You will see that LD_PRELOAD was not unset (even in the context of the
> > ctor function), and LD_PRELOAD is set in the shell.
> >
> > From what I can tell, LD_PRELOAD is not special, this is true of other
> > environment variables. However this is not reproducible with simpler
> > programs - bash 5.2 is the one where I saw this happen first, and so
> > far the only program I know that has this issue.
>
> Are you sure it's reproducible with other programs? I suspect what's
> happening is that bash is using the optional third argument envp to
> main, not the actual current environment, as its source to derive the
> initial environment list.
>
> I don't see any way the issue you're describing could happen
> otherwise.
>
> > Let me know if I'm doing something very wrong or if there is an easier
> > reproduction, from scanning bash's source I don't think they're doing
> > anything strange (they initialize variables with the `char** envp`
> > passed to main).
>
> Yes, so it's exactly what I suspected.
>
> > I do notice in `dynlink.c` that the stack pointer passed to the
> > entrypoint by the loader is whatever was passed to the loader, not
> > accounting for any envp mutations made by calling the DT_INIT_ARRAY
> > functions. But I don't know if this is actually a problem or not for
> > the implementation of _start.
>
> No application code, not even DT_INIT_ARRAY handlers, has executed at
> this point. It all runs after execution is passed to the main
> program's ELF entry point (_start).
>
> The behavior you're seeing has nothing to do with the dynamic linker.
> It's line 95 of src/env/__libc_start_main.c passing a pointer to the
> initial environment vector rather than any potentially-updated value
> of environ. I'm not sure if this should be changed or not (there are
> probably arguments for either), but bash using the nonstandard
> third-arg to main rather than the standard global environ[] strikes me
> as a bash bug.
>
> Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-10-01 12:08   ` Rich Felker
@ 2025-11-19 16:50     ` Markus Wichmann
  2025-11-19 17:09       ` Rich Felker
  0 siblings, 1 reply; 7+ messages in thread
From: Markus Wichmann @ 2025-11-19 16:50 UTC (permalink / raw)
  To: musl; +Cc: Mike Hilgendorf

Hi all,

it's been a while since this thread, but I still want to get back to it:
Is it sensible to pass __environ as third argument to main(), rather
than the original envp?

Reason I ask is that I recently checked out what the current POSIX says
about this third argument, and I found it doesn't say anything at all.
According to POSIX, this argument is a pure extension. So basically, we
can do what we want there. I seem to remember that earlier versions had
something to say about it, though, but it has been dropped from the
current description of exec().

Then I also checked out the bad actor in this thread, namely bash. I
wondered whether they are somehow testing for the presence of the third
arg. I struggle to think of a way to do so, however. If the third arg is
not given, then declaring one will in practice give garbage values that
can be anything. And there is absolutely no way to detect this before
runtime, making it a poor option for configure testing, since it isn't
cross-compilation compatible.

But anyway, what bash is doing is just detecting the platforms it knows
not to give that argument (namely OpenNT and MVS), and then it adds the
macro to switch the behavior to the command line. And in the code
itself, while it uses the NO_MAIN_ENV_ARG macro to select the behavior
in two places, in a third it just uses the platform macros directly. So
this whole thing just doesn't work if any implementation ever decides to
change its behavior (e.g. to align with POSIX).

Anyway, back to musl: Since it is currently giving a third arg to main,
stopping doing so would be an ABI breakage. But nobody said the third
argument has to be the original envp. If we pass __environ instead, then
the effects of constructors modifying the environment will show up in
main(). And __environ is definitely a valid environment (i.e. a vector
of pointers to strings, each being of the form "key=value"). So there's
no violation of any expectations.

Ciao,
Markus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function
  2025-11-19 16:50     ` Markus Wichmann
@ 2025-11-19 17:09       ` Rich Felker
  0 siblings, 0 replies; 7+ messages in thread
From: Rich Felker @ 2025-11-19 17:09 UTC (permalink / raw)
  To: Markus Wichmann; +Cc: musl, Mike Hilgendorf

On Wed, Nov 19, 2025 at 05:50:17PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> it's been a while since this thread, but I still want to get back to it:
> Is it sensible to pass __environ as third argument to main(), rather
> than the original envp?
> 
> Reason I ask is that I recently checked out what the current POSIX says
> about this third argument, and I found it doesn't say anything at all.
> According to POSIX, this argument is a pure extension. So basically, we
> can do what we want there. I seem to remember that earlier versions had
> something to say about it, though, but it has been dropped from the
> current description of exec().
> 
> Then I also checked out the bad actor in this thread, namely bash. I
> wondered whether they are somehow testing for the presence of the third
> arg. I struggle to think of a way to do so, however. If the third arg is
> not given, then declaring one will in practice give garbage values that
> can be anything. And there is absolutely no way to detect this before
> runtime, making it a poor option for configure testing, since it isn't
> cross-compilation compatible.
> 
> But anyway, what bash is doing is just detecting the platforms it knows
> not to give that argument (namely OpenNT and MVS), and then it adds the
> macro to switch the behavior to the command line. And in the code
> itself, while it uses the NO_MAIN_ENV_ARG macro to select the behavior
> in two places, in a third it just uses the platform macros directly. So
> this whole thing just doesn't work if any implementation ever decides to
> change its behavior (e.g. to align with POSIX).
> 
> Anyway, back to musl: Since it is currently giving a third arg to main,
> stopping doing so would be an ABI breakage. But nobody said the third
> argument has to be the original envp. If we pass __environ instead, then
> the effects of constructors modifying the environment will show up in
> main(). And __environ is definitely a valid environment (i.e. a vector
> of pointers to strings, each being of the form "key=value"). So there's
> no violation of any expectations.

I'm not strongly against making a change here, but also question
whether we want to be making changes to facilitate what's clearly
"doing something in a way that's clearly gratuitously nonportable and
wrong". Bash should be fixed to use environ whenever it exists and
only use the legacy envp arg to main on systems that somehow lack
environ.

It's not that it would be wrong to make this work, but changing
something to make it work, in my mind, implies some obligation to
*care that it works*, and in particular to deal with any fallout of
the change or other unexpected aspects of the functionality that's now
"intended to be well supported". That's something I think we'd be
better off not getting into without good motivation.

BTW note that, on implementations where C++ global ctors are
implemented via the C++ compiler emitting a call to the top of main,
the behavior will match what musl currently does: receiving the
original environment pointer from prior to ctor execution. So I think
applications using this need to be prepared for either behavior -- or
better yet, stop using it.

Rich

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-11-19 17:09 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-30 21:23 [musl] `unsetenv()` does not always work when run in an `__attribute__((constructor))` function Mike Hilgendorf
2025-10-01  7:44 ` Alexander Monakov
2025-10-01 12:06 ` Rich Felker
2025-10-01 12:08   ` Rich Felker
2025-11-19 16:50     ` Markus Wichmann
2025-11-19 17:09       ` Rich Felker
2025-10-01 13:49   ` Mike Hilgendorf

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).