[RP] putenv(), setenv(), unsetenv() (was: Fix for ratpoison OpenBSD-Port)

[Matthias Kilian]

[Note: there's a patch below, so read first before replying]

On Fri, Oct 08, 2004 at 08:53:06PM +0200, Matthias Kilian wrote:
> > I'll take a look at some other software and see what they do...I've
> > always been confused about putenv. It's time to get it straight.
> 
> Indeed. I'll discuss that with some friends. At least, according to

O.k., there were some different opinions; some BSD guy just ask why to
care about the IEEE Std 1003.1 definition of putenv(3) when putenv(3)
historically came from 4.3 BSD-Reno :-)

The overall picture seems to be:

- use setenv()/unsetenv() whenever possible

- use putenv() only when the POSIX semantics are required (aliasing
  instead of copying the string) or if you're *really* tight on memory
  and just want to setup an environment using static variables. For
  example:

  static char *foo = "FOO=42";
  static char *bar = "BAR=23";

  putenv(foo);
  putenv(bar);


For ratpoison, I wonder wether there's *any* system that doesn't have
setenv()/unsetenv() but does have putenv() with POSIX semantics.

The alternative would be to #define BROKEN_PUTENV for BSDs and then add 

#ifdef BROKEN_PUTENV
        free(foo);
#endif

where appropriate. IMHO, that's a real mess :-(

> I'll also have a deeper dive into the ratpoison sources to find a good
> solution.

Fortunately, putenv(3) is only used at four places in actions.c, and
seems that the aliasing semantics (i.e. change the environemnt *after*
the call to putenv()) aren't used at all.

So, I've hacked around a little bit and would like to suggest the
patch below. It contains the following changes:

1. src/screen.c: Don't store "DISPLAY=<DPY>" in s->display_string, just
   store "<DPY">.

2. src/actions.c: use setenv(name, valiue, 1) resp. unsetenv(name)
   instead of putenv("NAME=VALUE") resp. putenv("NAME=").

3. src/actions.c: for cmd_setenv(), use two auto variables, name and
   value, instead of just token, for better readability. I also changed
   the delimiter in the second strtok() from "\0" to "". If that
   explicit \0 was some kind of workaround, ignore it.

4. src/actions.c: in cmd_getenv(), fixed yet another memory leak.

5. src/actions.c: in cmd_unsetenv(), check for *exactly* one argument.

I've tested it on Linux, and I'll test it on OpenBSD later this day.


Two final issues: first, I'm not sure wether the duplication of the data
arguments in many of the cmd_XXX() functions is really required. For
example, cmd_getenv() seems to be unneccesary complicated IMHO. Second,
strtok() is marked as deprecated in many manpages and may be better
replaced by something other mechanism. Even better would be to tokenize
the arguments in the caller and change the func field in struct
user_command to char * (*func)(int, char **). That would probably
require an additional quoting implementation for ratpoison, however.

If you're interested, I will have a look for that two things and do a
little bit more hacking.

Ciao,
        Kili


Index: src/actions.c
===================================================================
RCS file: /cvsroot/ratpoison/ratpoison/src/actions.c,v
retrieving revision 1.203
diff -u -r1.203 actions.c
--- src/actions.c       8 Oct 2004 00:41:15 -0000       1.203
+++ src/actions.c       10 Oct 2004 13:13:07 -0000
@@ -1350,7 +1350,7 @@
     {
       /* Some process setup to make sure the spawned process runs
         in its own session. */
-      putenv(current_screen()->display_string);
+      setenv("DISPLAY", current_screen()->display_string, 1);
 #ifdef HAVE_SETSID
       setsid();
 #endif
@@ -1395,7 +1395,7 @@
 
   PRINT_DEBUG (("Switching to %s\n", prog));
 
-  putenv(current_screen()->display_string);
+  setenv("DISPLAY", current_screen()->display_string, 1);
   execlp(prog, prog, 0);
 
   PRINT_ERROR (("exec %s ", prog));
@@ -2639,8 +2639,8 @@
 char *
 cmd_setenv (int interactive, char *data)
 {
-  char *token, *dup;
-  struct sbuf *env;
+  char *name, *value;
+  char *dup;
 
   if (data == NULL)
     {
@@ -2648,45 +2648,32 @@
       return NULL;
     }
 
-  /* Setup the environment string. */
-  env = sbuf_new(0);
-
   /* Get the 2 arguments. */
   dup = xstrdup (data);
-  token = strtok (dup, " ");
-  if (token == NULL)
+  name = strtok (dup, " ");
+  if (name == NULL)
     {
       message (" setenv: two arguments required ");
       free (dup);
-      sbuf_free (env);
       return NULL;
     }
-  sbuf_concat (env, token);
-  sbuf_concat (env, "=");
 
-  token = strtok (NULL, "\0");
-  if (token == NULL)
+  value = strtok (NULL, "");
+  if (value == NULL)
     {
       message (" setenv: two arguments required ");
       free (dup);
-      sbuf_free (env);
       return NULL;
     }
-  sbuf_concat (env, token);
-
-  free (dup);
 
   /* Stick it in the environment. */
-  PRINT_DEBUG(("%s\n", sbuf_get(env)));
-  putenv (sbuf_get (env));
+  PRINT_DEBUG(("%s, %s\n", name, value));
+  if (setenv(name, value, 1))
+    {
+      marked_message_printf (0, 0, " setenv: %s : %s ", data, strerror(errno));
+    }
 
-  /* 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);
+  free (dup);
 
   return NULL;
 }
@@ -2718,6 +2705,7 @@
   if (interactive)
     {
       marked_message_printf (0, 0, " %s ", value);
+      free (var);
       return NULL;
     }
 
@@ -2727,6 +2715,8 @@
       strcpy (result, getenv (var));
     }
 
+  free (var);
+
   return result;
 }
 
@@ -2760,23 +2750,13 @@
 char *
 cmd_unsetenv (int interactive, char *data)
 {
-  struct sbuf *s;
-  char *str;
-
-  if (data == NULL)
+  if (data == NULL || strchr(data, ' ') != NULL)
     {
       message (" unsetenv: one argument is required ");
       return NULL;
     }
 
-  /* Remove all instances of the env. var. We must add an '=' for it
-     to work on OpenBSD. */
-  s = sbuf_new(0);
-  sbuf_copy (s, data);
-  sbuf_concat (s, "=");
-/*   str = sbuf_free_struct (s); */
-  putenv (sbuf_get(s));
-  sbuf_free (s);
+  unsetenv(data);
   return NULL;
 }
 
Index: src/screen.c
===================================================================
RCS file: /cvsroot/ratpoison/ratpoison/src/screen.c,v
retrieving revision 1.7
diff -u -r1.7 screen.c
--- src/screen.c        5 Oct 2004 03:07:22 -0000       1.7
+++ src/screen.c        10 Oct 2004 13:13:07 -0000
@@ -259,7 +259,7 @@
 
   /* Build the display string for each screen */
   s->display_string = xmalloc (strlen(DisplayString (dpy)) + 21);
-  sprintf (s->display_string, "DISPLAY=%s", DisplayString (dpy));
+  strcpy (s->display_string, DisplayString (dpy));
   if (strrchr (DisplayString (dpy), ':'))
     {
       char *dot;