diff mbox

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

Message ID 55062712.4040104@cs.ucla.edu
State Superseded
Headers show

Commit Message

Paul Eggert March 16, 2015, 12:42 a.m. UTC
Paul Pluzhnikov wrote:
> So it looks to me like the 2ecccaede9097f867284d352a881d8f226ba4fb7 is
> quite broken, and should be reverted.

I reverted it.  Sorry about that; it had a horrible typo (!= vs ==).  Does the 
attached (untested) patch work for you instead?  It fixes the typo, and also 
pacifies GCC so that GCC does not issue the bogus warning.

Comments

Paul Pluzhnikov March 16, 2015, 1:10 a.m. UTC | #1
On Sun, Mar 15, 2015 at 5:42 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> I reverted it.

Thanks,

> Sorry about that; it had a horrible typo (!= vs ==).

Oh, right. I should have noticed.

> Does
> the attached (untested) patch work for you instead?  It fixes the typo, and
> also pacifies GCC so that GCC does not issue the bogus warning.

I've tested the patch on Linux/x86_64. Looks good.

+/* Pacify GCC; see the commentary about VALLEN below.  This is needed
+   at least through GCC 4.9.2.  Pacify GCC for the entire file, as
+   there seems to be no way to pacify GCC selectively, only for the
+   place where it's needed.  Do not use DIAG_IGNORE_NEEDS_COMMENT
+   here, as it's not defined yet.  */
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"

You can be more selective with '#pragma GCC diagnostic push/pop'
around the function.

Clearly you know about DIAG_IGNORE_NEEDS_COMMENT (I didn't), so I
assume you also know about DIAG_PUSH_NEEDS_COMMENT /
DIAG_POP_NEEDS_COMMENT, which then makes me not understand your
comment about selective pacification.
Paul Eggert March 16, 2015, 4:11 a.m. UTC | #2
Paul Pluzhnikov wrote:
> I assume you also know about DIAG_PUSH_NEEDS_COMMENT /
> DIAG_POP_NEEDS_COMMENT, which then makes me not understand your
> comment about selective pacification.

When I tried to use those macros, I still got the warning and the build still 
failed.  Part of the problem is that 'vallen' gets used as part of an argument 
to the __mempcpy/memcpy macros, and GCC decides that since 'vallen' is used in 
the macro body, it'll issue a warning since the macro body is outside the 
"scope" of the DIAG_PUSH* and DIAG_POP* brackets.  It's the classic problem of 
macro capture.

I couldn't even use plain DIAG_IGNORE_NEEDS_COMMENT (i.e., without 
DIAG_PUSH_NEEDS_COMMENT), because I needed to put the #pragma before including 
<errno.h> (the first standard include) -- otherwise I got the warning -- and 
DIAG_IGNORE_NEEDS_COMMENT isn't defined until after <errno.h> is included.

I was using GCC 4.9.2 20150212 (Red Hat 4.9.2-6), for what that's worth.
Paul Pluzhnikov March 16, 2015, 4:33 a.m. UTC | #3
On Sun, Mar 15, 2015 at 9:11 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> When I tried to use those macros, I still got the warning and the build
> still failed.
...
> I was using GCC 4.9.2 20150212 (Red Hat 4.9.2-6), for what that's worth.

I see. I reproduced that "bad behavior" using GCC trunk @r221169, but
it works as I expected using GCC 4.8.2-19ubuntu1. That seems like a
GCC mis-feature.
Joseph Myers March 16, 2015, 11:48 p.m. UTC | #4
On Sun, 15 Mar 2015, Paul Eggert wrote:

> I couldn't even use plain DIAG_IGNORE_NEEDS_COMMENT (i.e., without
> DIAG_PUSH_NEEDS_COMMENT), because I needed to put the #pragma before including
> <errno.h> (the first standard include) -- otherwise I got the warning -- and
> DIAG_IGNORE_NEEDS_COMMENT isn't defined until after <errno.h> is included.

Any file using those macros should explicitly include <libc-internal.h> to 
get their definitions.  Are you saying that if you include 
<libc-internal.h>, then use the macros, then include other headers, that 
it doesn't work?
Paul Pluzhnikov April 5, 2015, 11:40 p.m. UTC | #5
On Sun, Mar 15, 2015 at 6:10 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
> On Sun, Mar 15, 2015 at 5:42 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> I've tested the patch on Linux/x86_64. Looks good.

The patch appears to have stalled.
I believe we still want it: the current situation is really not good.

Thanks,
Paul Eggert April 6, 2015, 2:43 a.m. UTC | #6
Paul Pluzhnikov wrote:

> The patch appears to have stalled.
> I believe we still want it: the current situation is really not good.

Yes.  Since that patch 
<https://sourceware.org/ml/libc-alpha/2015-03/msg00527.html> fixes a bug and has 
been reviewed and tested, I would like to install it now.  We can install any 
further improvements to libc-internal.h later.
Paul Eggert April 19, 2015, 8:09 a.m. UTC | #7
Paul Eggert wrote:
> Since that patch
> <https://sourceware.org/ml/libc-alpha/2015-03/msg00527.html> fixes a bug and has
> been reviewed and tested, I would like to install it now.

I've done that.  Thanks for reminding me about it.
diff mbox

Patch

From 7d298c5558df36cf0e6a46940ace042c52284264 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 15 Mar 2015 17:38:10 -0700
Subject: [PATCH] * stdlib/setenv.c (__add_to_environ):

Dump core quickly if setenv (..., NULL, ...) is called.
This time, do it the right way, and pacify GCC with a pragma.
---
 ChangeLog       |  4 ++++
 stdlib/setenv.c | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index c856f79..e61cc17 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@ 
 2015-03-15  Paul Eggert  <eggert@cs.ucla.edu>
 
+	* stdlib/setenv.c (__add_to_environ):
+	Dump core quickly if setenv (..., NULL, ...) is called.
+	This time, do it the right way, and pacify GCC with a pragma.
+
 	* stdlib/setenv.c (__add_to_environ): Revert previous change.
 
 2015-03-14  Andreas Schwab  <schwab@linux-m68k.org>
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index b60c4f0..184a8cd 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -19,6 +19,13 @@ 
 # include <config.h>
 #endif
 
+/* Pacify GCC; see the commentary about VALLEN below.  This is needed
+   at least through GCC 4.9.2.  Pacify GCC for the entire file, as
+   there seems to be no way to pacify GCC selectively, only for the
+   place where it's needed.  Do not use DIAG_IGNORE_NEEDS_COMMENT
+   here, as it's not defined yet.  */
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+
 #include <errno.h>
 #if !_LIBC
 # if !defined errno && !defined HAVE_ERRNO_DECL
@@ -114,8 +121,17 @@  __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 null (unfortunately GCC is not smart enough to deduce
+     this; see the #pragma at the start of this file).  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