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 <[email protected]> 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
