diff mbox

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

Message ID 55021AB7.1060905@cs.ucla.edu
State Superseded
Headers

Commit Message

Paul Eggert March 12, 2015, 11:01 p.m. UTC 
  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

Carlos O'Donell March 13, 2015, 2:08 p.m. UTC | #1 
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.
Roland McGrath March 13, 2015, 5:04 p.m. UTC | #2 
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.
Paul Eggert March 13, 2015, 5:17 p.m. UTC | #3 
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.
Jeff Law March 13, 2015, 5:35 p.m. UTC | #4 
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
Paul Pluzhnikov March 15, 2015, 10:54 p.m. UTC | #5 
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
Paul Pluzhnikov March 15, 2015, 10:57 p.m. UTC | #6 
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).
Paul Pluzhnikov March 15, 2015, 11:33 p.m. UTC | #7 
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.
diff mbox

Patch

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(-)

diff --git a/ChangeLog b/ChangeLog
index d6526e9..56b3d73 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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.
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index b60c4f0..0534236 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -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