Error on setenv(..., NULL, ...)
Commit Message
On 03/12/2015 02:53 PM, Roland McGrath wrote:
> The only reason I can see not
> to do that is to force the fault to be before the lock is taken. If that
> is the explicit intent of the code, then its comments should say so.
I had assumed that lengths are computed before locking to minimize the
size of the critical section, making it less of a bottleneck. Attached
is a revised (untested) patch with comments saying that.
Comments
On 03/12/2015 07:01 PM, Paul Eggert wrote:
> On 03/12/2015 02:53 PM, Roland McGrath wrote:
>> The only reason I can see not
>> to do that is to force the fault to be before the lock is taken. If that
>> is the explicit intent of the code, then its comments should say so.
>
> I had assumed that lengths are computed before locking to minimize
> the size of the critical section, making it less of a bottleneck.
> Attached is a revised (untested) patch with comments saying that.
Agreed.
This looks good to me. I like the fail early, and it follows the
project's desire to make that visible to users and a debugger at
the point you make the call.
Cheers,
Carlos.
That is fine by me. I'm a bit surprised the compiler actually figures out
that VALLEN cannot be used uninitialized and doesn't emit the warning.
But great if so.
Roland McGrath wrote:
> I'm a bit surprised the compiler actually figures out
> that VALLEN cannot be used uninitialized and doesn't emit the warning.
> But great if so.
Yes, GCC got smarter about that sort of thing quite some time ago -- I don't
remember exactly when. If this causes a problem with older GCCs I suppose
someone will let us know. In the meantime I installed the patch.
On 03/13/15 11:04, Roland McGrath wrote:
> That is fine by me. I'm a bit surprised the compiler actually figures out
> that VALLEN cannot be used uninitialized and doesn't emit the warning.
> But great if so.
I haven't looked deeply at this example, but we're consistently looking
to improve GCC's ability to prove certain edges in the control flow
graph are not executable.
Of particular interest are equivalences that are only valid on
particular paths. I can speculate that later in this code there's a use
of VALLEN that is conditional on the value of COMBINED.
Through block copying and simplifications we can isolate the paths
leaving the first combined != NULL test. On one of those paths there'll
be the second combined != NULL test which becomes redundant (ie known
static result). We eliminate the redundant test, simplify the CFG and
as a result the uninitialized use is removed and ultimately better code.
Jeff
On Fri, Mar 13, 2015 at 10:17 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> Roland McGrath wrote:
>>
>> I'm a bit surprised the compiler actually figures out
>> that VALLEN cannot be used uninitialized and doesn't emit the warning.
>> But great if so.
>
>
> Yes, GCC got smarter about that sort of thing quite some time ago
Which GCC did you use?
Using gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2 (latest "system" gcc on
Ubuntu 14.04), I get:
setenv.c: In function ‘__add_to_environ’:
setenv.c:176:17: error: ‘vallen’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
const size_t varlen = namelen + 1 + vallen;
^
cc1: all warnings being treated as errors
On Sun, Mar 15, 2015 at 3:54 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Fri, Mar 13, 2015 at 10:17 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>> Roland McGrath wrote:
>>>
>>> I'm a bit surprised the compiler actually figures out
>>> that VALLEN cannot be used uninitialized and doesn't emit the warning.
>>> But great if so.
>>
>>
>> Yes, GCC got smarter about that sort of thing quite some time ago
>
> Which GCC did you use?
>
> Using gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2 (latest "system" gcc on
> Ubuntu 14.04), I get:
>
> setenv.c: In function ‘__add_to_environ’:
> setenv.c:176:17: error: ‘vallen’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> const size_t varlen = namelen + 1 + vallen;
> ^
> cc1: all warnings being treated as errors
Hmm, I also get the same warning using quite recent trunk GCC build (@r221169).
On Sun, Mar 15, 2015 at 3:57 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> Hmm, I also get the same warning using quite recent trunk GCC build (@r221169).
I temporarily fixed the warning with:
@@ -121,7 +121,7 @@ __add_to_environ (name, value, combined, replace)
causes setenv (..., NULL, ...) to dump core now instead of
corrupting memory later. */
const size_t namelen = strlen (name);
- size_t vallen;
+ size_t vallen = 0;
if (combined != NULL)
vallen = strlen (value) + 1;
"make check" then fails a lot of tests, including stdlib/tst-environ,
which fails like so:
getenv #2 failed
getenv #3 failed
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106 movdqu (%rax), %xmm12
(gdb) bt
#0 strlen () at ../sysdeps/x86_64/strlen.S:106
#1 0x00007ffff7c924de in __add_to_environ (name=0x7fffffffe630
"FOOBAR", value=value@entry=0x0, combined=combined@entry=0x6031a0
"FOOBAR=some longer value", replace=replace@entry=1) at setenv.c:126
#2 0x00007ffff7c9242d in putenv (string=0x6031a0 "FOOBAR=some longer
value") at putenv.c:78
So it looks to me like the 2ecccaede9097f867284d352a881d8f226ba4fb7 is
quite broken, and should be reverted.
From 1441ea7a5cfb78e36c8e6f170a30de52fd01920e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 12 Mar 2015 15:57:07 -0700
Subject: [PATCH] * stdlib/setenv.c (__add_to_environ):
Dump core quickly if setenv (..., NULL, ...) is called.
---
ChangeLog | 5 +++++
stdlib/setenv.c | 10 +++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
@@ -1,3 +1,8 @@
+2015-03-12 Paul Eggert <eggert@cs.ucla.edu>
+
+ * stdlib/setenv.c (__add_to_environ):
+ Dump core quickly if setenv (..., NULL, ...) is called.
+
2015-03-12 Joseph Myers <joseph@codesourcery.com>
* soft-fp/soft-fp.h (_FP_STATIC_ASSERT): New macro.
@@ -114,8 +114,16 @@ __add_to_environ (name, value, combined, replace)
{
char **ep;
size_t size;
+
+ /* Compute lengths before locking, so that the critical section is
+ less of a performance bottleneck. VALLEN is needed only if
+ COMBINED is non-null. Also, testing COMBINED instead of VALUE
+ causes setenv (..., NULL, ...) to dump core now instead of
+ corrupting memory later. */
const size_t namelen = strlen (name);
- const size_t vallen = value != NULL ? strlen (value) + 1 : 0;
+ size_t vallen;
+ if (combined != NULL)
+ vallen = strlen (value) + 1;
LOCK;
--
2.1.0