[PATCH] environment handling fixes

From eea7d29e41ad5cad2934752bb07cdaa484fe363a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Courr=C3=A8ges-Anglas?= <[email protected]>
Date: Mon, 17 Dec 2012 06:48:25 +0100
Subject: [PATCH] environment handling fixes

* AC_CHECK_FUNCS: +setenv +unsetenv -putenv
  (the check for putenv() wasn't used anyway)
* prefer setenv() to putenv() in cmd_setenv()
* prefer unsetenv() to putenv() in cmd_unsetenv()
  - putenv("FOO") isn't legit everywhere
  - putenv("FOO=") will only work on MinGW
* make the getenv command return an empty output if the variable wasn't found
* while here, split and sort AC_CHECK_FUNCS

|cos| on #ratpoison reported that environment variables weren't
properly removed, and proposed a different fix.  thanks!
---
 configure.in  |  3 ++-
 src/actions.c | 63 +++++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/configure.in b/configure.in
index 08f4ee8..b60e103 100644
--- a/configure.in
+++ b/configure.in
@@ -146,7 +146,8 @@ AC_CHECK_HEADERS(unistd.h stdarg.h)
 dnl Checks for typedefs, structures, and compiler characteristics.
 
 dnl Checks for library functions.
-AC_CHECK_FUNCS(getopt getopt_long setsid setpgid setpgrp putenv vsnprintf usleep getline)
+AC_CHECK_FUNCS(getline getopt getopt_long setenv setpgid setpgrp setsid)
+AC_CHECK_FUNCS(unsetenv usleep vsnprintf)
 
 AC_TYPE_SIGNAL
 
diff --git a/src/actions.c b/src/actions.c
index bf381c8..dafe836 100644
--- a/src/actions.c
+++ b/src/actions.c
@@ -4125,28 +4125,36 @@ set_bwcolor (struct cmdarg **args)
 cmdret *
 cmd_setenv (int interactive UNUSED, struct cmdarg **args)
 {
+
+  const char *var = ARG_STRING(0), *val = ARG_STRING(1);
+  int ret;
+
+#ifdef HAVE_SETENV
+
+  PRINT_DEBUG(("setenv (\"%s\", \"%s\", 1)\n", var, val));
+  ret = setenv (var, val, 1);
+
+#else /* not HAVE_SETENV */
+
   struct sbuf *env;
 
   /* Setup the environment string. */
-  env = sbuf_new(0);
+  env = sbuf_new (strlen (var) + 1 + strlen (val) + 1);
 
-  sbuf_concat (env, ARG_STRING(0));
+  sbuf_copy (env, var);
   sbuf_concat (env, "=");
-  sbuf_concat (env, ARG_STRING(1));
+  sbuf_concat (env, val);
 
-  /* Stick it in the environment. */
-  PRINT_DEBUG(("%s\n", sbuf_get(env)));
-  putenv (sbuf_get (env));
+  /* Stick it in the environment.  We must *not* free it. */
+  PRINT_DEBUG(("putenv(\"%s\")\n", sbuf_get(env)));
+  ret = putenv (sbuf_free_struct (env));
 
-  /* According to the docs, the actual string is placed in the
-     environment, not the data the string points to. This means
-     modifying the string (or freeing it) directly changes the
-     environment. So, don't free the environment string, just the sbuf
-     data structure. */
-  env->data = NULL;
-  sbuf_free (env);
+#endif /* not HAVE_SETENV */
 
-  return cmdret_new (RET_SUCCESS, NULL);
+  if (ret == -1)
+    return cmdret_new (RET_FAILURE, "cmd_setenv failed: %s", strerror(errno));
+  else
+    return cmdret_new (RET_SUCCESS, NULL);
 }
 
 cmdret *
@@ -4158,7 +4166,7 @@ cmd_getenv (int interactive UNUSED, struct cmdarg **args)
   if (value)
     return cmdret_new (RET_SUCCESS, "%s", value);
   else
-    return cmdret_new (RET_SUCCESS, "");
+    return cmdret_new (RET_SUCCESS, NULL);
 }
 
 /* Thanks to Gergely Nagy <[email protected]> for the original
@@ -4190,16 +4198,25 @@ cmd_chdir (int interactive UNUSED, struct cmdarg **args)
 cmdret *
 cmd_unsetenv (int interactive UNUSED, struct cmdarg **args)
 {
-  struct sbuf *s;
+  const char *var = ARG_STRING(0);
+  int ret;
 
-  /* Remove all instances of the env. var. We must add an '=' for it
-     to work on OpenBSD. */
-  s = sbuf_new(0);
-  sbuf_copy (s, ARG_STRING(0));
+  /* Use unsetenv() where possible since putenv("FOO") is not legit everywhere */
+#ifdef HAVE_UNSETENV
+  ret = unsetenv(var);
+#else
+  /* MinGW doesn't have unsetenv() and uses putenv("FOO=") */
+  struct sbuf *s = sbuf_new (strlen (var) + 1 + 1);
+  sbuf_copy (s, var);
   sbuf_concat (s, "=");
-  putenv (sbuf_get(s));
-  sbuf_free (s);
-  return cmdret_new (RET_SUCCESS, NULL);
+  /* let putenv() decide whether to call free() */
+  ret = putenv (sbuf_free_struct (s));
+#endif
+
+  if (ret == -1)
+    return cmdret_new (RET_FAILURE, "cmd_unsetenv failed: %s", strerror (errno));
+  else
+    return cmdret_new (RET_SUCCESS, NULL);
 }
 
 /* Thanks to Gergely Nagy <[email protected]> for the original
-- 
1.8.0.1

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (OpenBSD)

iQIcBAEBCgAGBQJQzrtSAAoJEGGRj78GoRSUZd8P/iJQuDuuiHcMjcoBKoZtqdlP
ucPVxlyN8o9D3RJqsD70RQEJC/PhNNLYq7i5n7zONKbAknJKjxeHmEMJk+0wU6kT
jrB75hK1DQcc5gZn04Qa3TTwUO23ylnzZep1E54dAyWJQp+m1lYovJ7cA4EpinxY
Afh1NMSg9zPjyg2KRKRMSpK0D+ZcwQ9oIAoqUADm36vgf4udw++5JUMpI/QtSaWq
MrKnwDEXe74l/XDN5fsjpXc2QsvTs/5l2BL0EC31JyGYTilg4ta43e/yDOvwEVQE
f/dAgfTHtHrD6+Vd49MAWXGMgZZXXDUM6IoFD+QPQ8uqPxWC6AMA7Wa9MIrKkZll
6RJ7QXfMQ5a9dIntwRbaJjAwWctxJV594whwz9fr1FteQ1EwNsighaKxIJizuREp
L92nj4t58fEWerjekpl04Vrh/W7jaGFs/Jk6kaQvfQ5C+LRN+slvKgpcLJ9BCeql
1yD3wmX6wsammdBsWngognrN0RLk3r+07GaT+K+mBvsZFzczWSaUy0zcEUOpqtWt
9xzxbZD4hOK2EeJPPACB0morE4MD9wqVFKsESW8vjQH+0X7NYgkagJcEbc3Ix6qI
QsW5hpUI0OO2dvfyxdzjaGr7RE7eUjk5j7o8NOUfeOxAS7P8tSpq1NJ9uolal+Hs
GH3UOUCjW05IUY5wmOM/
=Y4Ij
-----END PGP SIGNATURE-----