Message ID | CALoOobNSbWUkd_i-L6U0ovbqPYnJY-h=ftX1K61yb19pmJj6aw@mail.gmail.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 95437 invoked by alias); 11 Mar 2015 16:12:38 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 95027 invoked by uid 89); 11 Mar 2015 16:12:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-ob0-f178.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to :content-type; bh=I6xMh1vY3awlhJtAHB70+fXFSm2YmVsVJBYvy1h+lT8=; b=DYAiQFNbzhmbvepD1uTSGR1A9IlY/MhX0Uuzt/K1RX+eVvYVL7O+ALs9h2ZXG/UnDI L5DT6uXmwngxdSwSjC0hkPm19cay7VRBVY9vhjOagwHqFvkCitcvt+GydK1OBCDkM0/d rEiDctd3u9Jzj/pj8yvTHWr3uoCWfbJXAkPlpRvmbBnk2UOo0OZQmi5hAWi1yJb++Fpx p2D+0SiTKtgAoZjxzlnRx/Hu4gvUb3IDR47Gwh66Zju20+qSYA1X9Pt82TWgRfdt+avP l1AyJOxAQ/02FkIX4ORlpxCFLdFEsapXxvDVvCnCUroAzJx+7LcL2DTfr8vetYXDkr9E ibzw== X-Gm-Message-State: ALoCoQna3igkdaZ9BTJPg0HYDnGtiGKIkYjF/iDKBZxPZ05Jl+6U9VU7XeU7eoMd3tu9HKq4cOiu X-Received: by 10.182.230.132 with SMTP id sy4mr30862205obc.29.1426090349579; Wed, 11 Mar 2015 09:12:29 -0700 (PDT) MIME-Version: 1.0 From: Paul Pluzhnikov <ppluzhnikov@google.com> Date: Wed, 11 Mar 2015 09:11:59 -0700 Message-ID: <CALoOobNSbWUkd_i-L6U0ovbqPYnJY-h=ftX1K61yb19pmJj6aw@mail.gmail.com> Subject: [patch] Error on setenv(..., NULL, ...) To: GLIBC Devel <libc-alpha@sourceware.org> Content-Type: multipart/mixed; boundary=001a11c336d4b4844e0511058802 |
Commit Message
Paul Pluzhnikov
March 11, 2015, 4:11 p.m. UTC
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.
Comments
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
diff --git a/stdlib/setenv.c b/stdlib/setenv.c index b60c4f0..63a95cf 100644 --- a/stdlib/setenv.c +++ b/stdlib/setenv.c @@ -240,7 +240,8 @@ setenv (name, value, replace) const char *value; int replace; { - if (name == NULL || *name == '\0' || strchr (name, '=') != NULL) + if (name == NULL || *name == '\0' || strchr (name, '=') != NULL + || value == NULL) { __set_errno (EINVAL); return -1;