Error on setenv(..., NULL, ...)
Commit Message
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
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 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.
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.
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?
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 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 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.
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(-)
@@ -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>
@@ -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