Re: env -u behavior

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> Glad you agree.  How about this patch?
> 
> * src/env.c (main): Use unsetenv rather than putenv to remove
> items from environ, and check for failure.

Since there is a gnulib module for unsetenv, I've modified my proposed patch to 
pick it up.  Meanwhile, I noticed that we can kill some dead code.  (For that 
matter, stdbuf wasn't even modifying environ directly.)

By the way, the POSIX folks recently admitted that setenv() is better than 
putenv(), in that the former allows implementations to provide O(log n) 
searching (by maintaining env as a sorted array) or even O(1) searching (by 
maintaining a hidden counterpart hashtable) compared to the traditional O(n) 
behavior (of stepping through every array entry), if the implementation is 
allowed to assume that the user can't muck inappropriately with pointers 
reachable from raw environ.  But in the current wording of their ruling, they 
also stated that changing raw environ (as currently done in env.c and su.c) 
renders an application non-conforming, with behavior of subsequent functions 
(like execve) no longer being guaranteed.  In other words, when it is time to 
replace the environment, it is unportable to use the O(1) swap of setting 
environ=new_environ, and a compliant app must instead implement an O(n) clearenv
() wrapper (for systems where it is not already provided natively) which 
repeatedly calls unsetenv() for every name currently in the environment.
http://austingroupbugs.net/view.php?id=167
http://www.opengroup.org/austin/mailarchives/ag/msg44144.html

The Austin group also recently opened up their bug database for anonymous 
viewing, so the above link should no longer require you to register.
https://www.opengroup.org/sophocles/show_mail.tpl?
CALLER=index.tpl&source=L&listname=austin-group-l&id=12951

Personally, I'm not too worried about the Austin ruling - yes, our code in 
env.c and su.c is no longer strictly conforming, but until someone points out 
an actual system where directly resetting environ messes up their expected 
behavior, I'm not going to try to patch the code based on paranoia.  And I will 
be chiming in to the Austin group asking that swapping environ itself (which is 
different than altering contents reachable from environ) be supported by the 
standards, similar to Joerg Schilling's request:
http://www.opengroup.org/austin/mailarchives/ag/msg43077.html

At any rate, is this patch OK to apply?


From: Eric Blake <address@hidden>
Date: Mon, 26 Oct 2009 14:32:59 -0600
Subject: [PATCH] maint: let gnulib provide environ

* bootstrap.conf (gnulib_modules): Add environ.
* src/env.c (environ): Delete declaration.
* src/printenv.c (environ): Likewise.
* src/stdbuf.c (environ): Likewise.
* src/su.c (environ): Likewise.
---
 bootstrap.conf |    1 +
 src/env.c      |    2 --
 src/printenv.c |    2 --
 src/stdbuf.c   |    2 --
 src/su.c       |    2 --
 5 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 1857489..f26dfcb 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -72,6 +72,7 @@ gnulib_modules="
   dirfd
   dirname
   dup2
+  environ
   error
   euidaccess
   exclude
diff --git a/src/env.c b/src/env.c
index b69a29a..ee5e6b6 100644
--- a/src/env.c
+++ b/src/env.c
@@ -91,8 +91,6 @@
   proper_name ("Richard Mlynarik"), \
   proper_name ("David MacKenzie")

-extern char **environ;
-
 static struct option const longopts[] =
 {
   {"ignore-environment", no_argument, NULL, 'i'},
diff --git a/src/printenv.c b/src/printenv.c
index 8185da6..dcdcb83 100644
--- a/src/printenv.c
+++ b/src/printenv.c
@@ -45,8 +45,6 @@ enum { PRINTENV_FAILURE = 2 };
   proper_name ("David MacKenzie"), \
   proper_name ("Richard Mlynarik")

-extern char **environ;
-
 void
 usage (int status)
 {
diff --git a/src/stdbuf.c b/src/stdbuf.c
index 3930713..885356b 100644
--- a/src/stdbuf.c
+++ b/src/stdbuf.c
@@ -39,8 +39,6 @@

 static char *program_path;

-extern char **environ;
-
 static struct
 {
   size_t size;
diff --git a/src/su.c b/src/su.c
index 0de67c9..add100a 100644
--- a/src/su.c
+++ b/src/su.c
@@ -113,8 +113,6 @@

 char *crypt (char const *key, char const *salt);

-extern char **environ;
-
 static void run_shell (char const *, char const *, char **, size_t)
      ATTRIBUTE_NORETURN;

-- 
1.6.4.2