Error on setenv(..., NULL, ...)

  Greetings,

The following test program:

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

int main() {
  setenv("ZZZ", NULL, 1);
  char *p = getenv("ZZZ");
  printf("%c\n", p[0]);
  return 0;
}

produces "unusable" environment, in which getenv("ZZZ") succeeds, but
you can't look at any bytes of the resulting pointer:

gcc -g t.c
t.c: In function ‘main’:
t.c:5:3: warning: null argument where non-null required (argument 2) [-Wnonnull]
   setenv("ZZZ", NULL, 1);
   ^

valgrind ./a.out

==27832== Invalid read of size 1
==27832==    at 0x4005FB: main (/tmp/t.c:7)
==27832==  Address 0x4dea3e4 is 0 bytes after a block of size 4 alloc'd
==27832==    at 0x40307C4: malloc
(valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c:270)
==27832==    by 0x4A60C59: __add_to_environ
(/build/buildd/eglibc-2.19/stdlib/setenv.c:193)
==27832==    by 0x40344BF: setenv (valgrind/memcheck/mc_replace_strmem.c:1643)
==27832==    by 0x4005E8: main (/tmp/t.c:5)


See also https://sourceware.org/ml/libc-alpha/2015-03/msg00402.html,
where GLIBC performed the bad setenv() itself.

Attached trivial patch makes setenv(..., NULL, ...) fail instead of
producing "bad" environment. Tested on Linux/x86_64, no new failures.

Thanks,


2015-03-11  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * stdlib/setenv.c (setenv): Reject NULL value in setenv.
  

On Wed, Mar 11, 2015 at 09:11:59AM -0700, Paul Pluzhnikov wrote:
> Attached trivial patch makes setenv(..., NULL, ...) fail instead of
> producing "bad" environment. Tested on Linux/x86_64, no new failures.

The standard says[1]:

"The environment variable shall be set to the value to which envval points"

and is vague since it could be interpreted in one of two ways:

1. setenv (..., NULL, ...) is equivalent to unsetting the environment
   variable, since getenv(...) would return NULL.

2. setenv (..., NULL, ...) is equivalent to setenv (..., "", ...),
   resulting in getenv returning a blank value.

Making it fail would be incorrect since it could break programs that
did not previously expect a failure from an invalid value and will
also not conform to the standard since the standard does not specify
such a failure.  While Option 1 seems closer to the above statement to
me, Option 2 seems like a safer fix.

Siddhesh

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html
  

On 11/03/15 16:33, Siddhesh Poyarekar wrote:
> On Wed, Mar 11, 2015 at 09:11:59AM -0700, Paul Pluzhnikov wrote:
>> Attached trivial patch makes setenv(..., NULL, ...) fail instead of
>> producing "bad" environment. Tested on Linux/x86_64, no new failures.
> 
> The standard says[1]:
> 
> "The environment variable shall be set to the value to which envval points"
> 
> and is vague since it could be interpreted in one of two ways:
> 

no, this just says that NULL argument is undefined behaviour

this is not a bug in glibc and i don't think any change should be made

> 1. setenv (..., NULL, ...) is equivalent to unsetting the environment
>    variable, since getenv(...) would return NULL.
> 
> 2. setenv (..., NULL, ...) is equivalent to setenv (..., "", ...),
>    resulting in getenv returning a blank value.
> 
> Making it fail would be incorrect since it could break programs that
> did not previously expect a failure from an invalid value and will
> also not conform to the standard since the standard does not specify
> such a failure.  While Option 1 seems closer to the above statement to
> me, Option 2 seems like a safer fix.
> 
> Siddhesh
> 
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html
>
  

On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> no, this just says that NULL argument is undefined behaviour
> 
> this is not a bug in glibc and i don't think any change should be made

Fair enough, but if we ever decide to protect ourselves against such
bad access, I'd be in favour of something more conservative like
returning a blank string than returning an error.

Siddhesh
  

On Wed, 11 Mar 2015, Paul Pluzhnikov wrote:

> Attached trivial patch makes setenv(..., NULL, ...) fail instead of
> producing "bad" environment. Tested on Linux/x86_64, no new failures.

The conventions at 
<https://sourceware.org/glibc/wiki/Style_and_Conventions#Error_Handling> 
say that "If it's user code invoking undefined behavior, then it should 
fail early and catastrophically ... That too trades off against any 
runtime cost of detecting the case.".  And, more specifically for null 
pointers, "If you're going to check for NULL pointer arguments where you 
have not entered into a contract to accept and interpret them, do so with 
an assert, not a conditional error return.".

So, if it's undefined behavior to pass NULL here, any detection should 
take the form of an assertion.
  

On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > no, this just says that NULL argument is undefined behaviour
> > 
> > this is not a bug in glibc and i don't think any change should be made
> 
> Fair enough, but if we ever decide to protect ourselves against such
> bad access, I'd be in favour of something more conservative like
> returning a blank string than returning an error.

if we agree it's undefined behavior, then can't we have fortification turn this 
into a build failure ?
-mike
  

On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> if we agree it's undefined behavior, then can't we have fortification turn this 
> into a build failure ?

The compiler already warns about the non-null requirement where it can
find it, so making it fail shouldn't be hard.  Maybe we could add the
assert as Joseph suggests for FORTIFY_SOURCE.  I am not convinced that
it would be conservative enough to do fail catastrophically in the
default case, hence my suggestion to return a blank string instead of
the bad value.

Siddhesh
  

On Wed, Mar 11, 2015 at 10:14 AM, Joseph Myers <joseph@codesourcery.com> wrote:

> So, if it's undefined behavior to pass NULL here, any detection should
> take the form of an assertion.

Assert would be inconsistent with the other checks: it surely is just as
undefined to pass a NULL name.

I am fine with

  assert (name != NULL && *name != '\0' && value != NULL);
  if (strchr (name, '=') != NULL)
    {
      __set_errno (EINVAL);
      return -1;
    }
...

but this may turn out to be too drastic.

The original check was added here:


Author: Roland McGrath <roland@gnu.org>
Date:   Wed Jun 9 18:33:36 2004 +0000

    * sysdeps/generic/setenv.c (setenv): Return -1/EINVAL if name is

        NULL, "" or contains '=' character in it.  Reported by
        Michael T Kerrisk <mtk-lists@gmx.net>.
        * stdlib/tst-environ.c: Include errno.h.
        (main): Add tests for these arguments to setenv/unsetenv.


+cc Roland, Michael
Thread starts here: https://sourceware.org/ml/libc-alpha/2015-03/msg00449.html
  

On 11/03/15 17:40, Paul Pluzhnikov wrote:
> On Wed, Mar 11, 2015 at 10:14 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
>> So, if it's undefined behavior to pass NULL here, any detection should
>> take the form of an assertion.
> 
> Assert would be inconsistent with the other checks: it surely is just as
> undefined to pass a NULL name.
> 

that's not undefined, the name can be 0 and then EINVAL must be set.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html
  

On Wed, Mar 11, 2015 at 11:19 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:

>> Assert would be inconsistent with the other checks: it surely is just as
>> undefined to pass a NULL name.
>>
>
> that's not undefined, the name can be 0 and then EINVAL must be set.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html

Where does it say that NULL name is allowed?

  [EINVAL]
  The envname argument points to an empty string or points to a string
containing an '=' character.

I read that as:

  assert (name != NULL && value != NULL);
  if (*name == '\0' || strchr (name, '=') != NULL)
    ...
  

On 11/03/15 18:26, Paul Pluzhnikov wrote:
> On Wed, Mar 11, 2015 at 11:19 AM, Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> 
>>> Assert would be inconsistent with the other checks: it surely is just as
>>> undefined to pass a NULL name.
>>>
>>
>> that's not undefined, the name can be 0 and then EINVAL must be set.
>>
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html
> 
> Where does it say that NULL name is allowed?
> 
>   [EINVAL]
>   The envname argument points to an empty string or points to a string
> containing an '=' character.
> 

ah sorry i havent read it just posted the link, this seems to be
a change in posix 2013

here is the 2004 version:

http://pubs.opengroup.org/onlinepubs/009695399/functions/setenv.html

(i dont see the change in the change history which i think is
an editing bug in posix, 2008 had the 0 too, but it's no longer
available online only the updated 2013 version

anyway i think 0 name should be handled in the future because
applications depend on it)
  

On 11/03/15 18:36, Szabolcs Nagy wrote:
> (i dont see the change in the change history which i think is
> an editing bug in posix, 2008 had the 0 too, but it's no longer

ok now i see it:

http://austingroupbugs.net/view.php?id=185

(so i'll have to fix my libc testing code..)
  

On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> > On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > > no, this just says that NULL argument is undefined behaviour
> > > 
> > > this is not a bug in glibc and i don't think any change should be made
> > 
> > Fair enough, but if we ever decide to protect ourselves against such
> > bad access, I'd be in favour of something more conservative like
> > returning a blank string than returning an error.
> 
> if we agree it's undefined behavior, then can't we have fortification turn this 
> into a build failure ?

Not a build failure but a runtime trap. UB can't be caught at build
time because it's only forbidden if the statement that results in UB
is reached, and reachability is equivalent to the halting problem.

Rich
  

On 11 Mar 2015 14:42, Rich Felker wrote:
> On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> > On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> > > On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > > > no, this just says that NULL argument is undefined behaviour
> > > > 
> > > > this is not a bug in glibc and i don't think any change should be made
> > > 
> > > Fair enough, but if we ever decide to protect ourselves against such
> > > bad access, I'd be in favour of something more conservative like
> > > returning a blank string than returning an error.
> > 
> > if we agree it's undefined behavior, then can't we have fortification turn this 
> > into a build failure ?
> 
> Not a build failure but a runtime trap. UB can't be caught at build
> time because it's only forbidden if the statement that results in UB
> is reached, and reachability is equivalent to the halting problem.

i don't think that nuance matters to fortification.  what i'm talking about 
is when gcc proves a NULL is used.  it already has a nonull warning so it can 
detect this.
-mike
  

On Wed, Mar 11, 2015 at 03:23:55PM -0400, Mike Frysinger wrote:
> On 11 Mar 2015 14:42, Rich Felker wrote:
> > On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> > > On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> > > > On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > > > > no, this just says that NULL argument is undefined behaviour
> > > > > 
> > > > > this is not a bug in glibc and i don't think any change should be made
> > > > 
> > > > Fair enough, but if we ever decide to protect ourselves against such
> > > > bad access, I'd be in favour of something more conservative like
> > > > returning a blank string than returning an error.
> > > 
> > > if we agree it's undefined behavior, then can't we have fortification turn this 
> > > into a build failure ?
> > 
> > Not a build failure but a runtime trap. UB can't be caught at build
> > time because it's only forbidden if the statement that results in UB
> > is reached, and reachability is equivalent to the halting problem.
> 
> i don't think that nuance matters to fortification.  what i'm talking about 
> is when gcc proves a NULL is used.  it already has a nonull warning so it can 
> detect this.

But it can't prove the code is reached. For example:

	static volatile size_t foo = sizeof(long);
	if (foo == 8) {
		setenv(p, q, 0);
	}

Suppose here that p or q necessarily ends up being a null pointer on
32-bit archs. The code is not reachable, so it doesn't matter, but
because foo is volatile the compiler can't prove it's not reachable.

This is a stupid example using volatile to make the point, but there
are lots of other ways things like this can happen, especially with
non-trivial use of macros and inline functions where ipa might happen
to prove that a pointer is NULL but fail to prove that the relevant
code is unreachable.

Rich
  

On 11 Mar 2015 18:02, Rich Felker wrote:
> On Wed, Mar 11, 2015 at 03:23:55PM -0400, Mike Frysinger wrote:
> > On 11 Mar 2015 14:42, Rich Felker wrote:
> > > On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> > > > On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> > > > > On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > > > > > no, this just says that NULL argument is undefined behaviour
> > > > > > 
> > > > > > this is not a bug in glibc and i don't think any change should be made
> > > > > 
> > > > > Fair enough, but if we ever decide to protect ourselves against such
> > > > > bad access, I'd be in favour of something more conservative like
> > > > > returning a blank string than returning an error.
> > > > 
> > > > if we agree it's undefined behavior, then can't we have fortification turn this 
> > > > into a build failure ?
> > > 
> > > Not a build failure but a runtime trap. UB can't be caught at build
> > > time because it's only forbidden if the statement that results in UB
> > > is reached, and reachability is equivalent to the halting problem.
> > 
> > i don't think that nuance matters to fortification.  what i'm talking about 
> > is when gcc proves a NULL is used.  it already has a nonull warning so it can 
> > detect this.
> 
> But it can't prove the code is reached. For example:
> 
> 	static volatile size_t foo = sizeof(long);
> 	if (foo == 8) {
> 		setenv(p, q, 0);
> 	}
> 
> Suppose here that p or q necessarily ends up being a null pointer on
> 32-bit archs. The code is not reachable, so it doesn't matter, but
> because foo is volatile the compiler can't prove it's not reachable.
> 
> This is a stupid example using volatile to make the point, but there
> are lots of other ways things like this can happen, especially with
> non-trivial use of macros and inline functions where ipa might happen
> to prove that a pointer is NULL but fail to prove that the relevant
> code is unreachable.

again, i don't think reachable matters.  why do we care about trying to let bad 
code compile ?  if we let fortification make it a build-time assert, what valid 
code that we care about is broken ?
-mike
  

On Wed, Mar 11, 2015 at 06:22:31PM -0400, Mike Frysinger wrote:
> On 11 Mar 2015 18:02, Rich Felker wrote:
> > On Wed, Mar 11, 2015 at 03:23:55PM -0400, Mike Frysinger wrote:
> > > On 11 Mar 2015 14:42, Rich Felker wrote:
> > > > On Wed, Mar 11, 2015 at 01:25:46PM -0400, Mike Frysinger wrote:
> > > > > On 11 Mar 2015 22:14, Siddhesh Poyarekar wrote:
> > > > > > On Wed, Mar 11, 2015 at 04:38:09PM +0000, Szabolcs Nagy wrote:
> > > > > > > no, this just says that NULL argument is undefined behaviour
> > > > > > > 
> > > > > > > this is not a bug in glibc and i don't think any change should be made
> > > > > > 
> > > > > > Fair enough, but if we ever decide to protect ourselves against such
> > > > > > bad access, I'd be in favour of something more conservative like
> > > > > > returning a blank string than returning an error.
> > > > > 
> > > > > if we agree it's undefined behavior, then can't we have fortification turn this 
> > > > > into a build failure ?
> > > > 
> > > > Not a build failure but a runtime trap. UB can't be caught at build
> > > > time because it's only forbidden if the statement that results in UB
> > > > is reached, and reachability is equivalent to the halting problem.
> > > 
> > > i don't think that nuance matters to fortification.  what i'm talking about 
> > > is when gcc proves a NULL is used.  it already has a nonull warning so it can 
> > > detect this.
> > 
> > But it can't prove the code is reached. For example:
> > 
> > 	static volatile size_t foo = sizeof(long);
> > 	if (foo == 8) {
> > 		setenv(p, q, 0);
> > 	}
> > 
> > Suppose here that p or q necessarily ends up being a null pointer on
> > 32-bit archs. The code is not reachable, so it doesn't matter, but
> > because foo is volatile the compiler can't prove it's not reachable.
> > 
> > This is a stupid example using volatile to make the point, but there
> > are lots of other ways things like this can happen, especially with
> > non-trivial use of macros and inline functions where ipa might happen
> > to prove that a pointer is NULL but fail to prove that the relevant
> > code is unreachable.
> 
> again, i don't think reachable matters.  why do we care about trying to let bad 
> code compile ?  if we let fortification make it a build-time assert, what valid 
> code that we care about is broken ?

It's not bad code. Here's a different example showing where a
compile-time bounds check on memcpy would be invalid:

	long x;
	if (sizeof(long) == 8) memcpy(&x, buf, 8);

Such use of if instead of #if is considered idiomatic/correct by many
projects because it validates the syntax and other aspects of code
that will be dead code on the current target. Of course this example
is over-simplified; in practice, the overflow-in-unreachable-path
would come from more complex expressions.

This kind of thing can and will arise from non-trivial use of macros
or inline functions -- cases where the compiler fails to prove
non-reachability but succeeds in propagating a constant that would
trigger the compile-time error. This is not conforming behavior for
the compiler.

Rich