Overwrite tracked file descriptors when exporting COLORED_STDERR_FDS.

If the list of tracked file descriptors is modified to contain unwanted
values (e.g. when a parent process closes or redirects stderr), this
makes it possible to set the tracked file descriptors back to a given
list.

This problem occurred with startx which somehow closes or redirects
stderr. As COLORED_STDERR_FDS was set in the shell running startx, this
disabled coloring stderr for all processes started in the X11 session.

This is no longer an issue. As COLORED_STDERR_FDS is set in the shell
configuration file, it automatically sets the list to the correct value
when a shell is opened in the terminal emulator once X11 is running.

src/coloredstderr.c

@@ -78,6 +78,9 @@ static size_t (*real_fwrite)(void const *, size_t, size_t, FILE *);
 static int initialized;
 /* Force hooked writes even when not writing to a tty. Used for tests. */
 static int force_write_to_non_tty;
+/* Was ENV_NAME_FDS found and used when init_from_environment() was called?
+ * This is not true if the process set it manually after initialization. */
+static int used_fds_set_by_user;
 
 
 #include "constants.h"
@@ -508,34 +511,23 @@ pid_t vfork(void) {
 
 /* Hook execve() and the other exec*() functions. Some shells use exec*() with
  * a custom environment which doesn't necessarily contain our updates to
- * ENV_NAME_FDS. It's also faster to update the environment only when
- * necessary, right before the exec() to pass it to the new process. */
+ * ENV_NAME_PRIVATE_FDS. It's also faster to update the environment only when
+ * necessary, right before the exec(), to pass it to the new program. */
 
 /* int execve(char const *, char * const [], char * const []) */
 HOOK_FUNC_DEF3(int, execve, char const *, filename, char * const *, argv, char * const *, env) {
     DLSYM_FUNCTION(real_execve, "execve");
 
-    int found = 0;
-    size_t index = 0;
-
-    /* Count arguments and search for existing ENV_NAME_FDS environment
-     * variable. */
+    /* Count environment variables. */
     size_t count = 0;
     char * const *x = env;
-    while (*x) {
-        if (!strncmp(*x, ENV_NAME_FDS "=", strlen(ENV_NAME_FDS) + 1)) {
-            found = 1;
-            index = count;
-        }
-
-        x++;
+    while (*x++) {
         count++;
     }
     /* Terminating NULL. */
     count++;
 
     char *env_copy[count + 1 /* space for our new entry if necessary */];
-    memcpy(env_copy, env, count * sizeof(char *));
 
     /* Make sure the information from the environment is loaded. We can't just
      * do nothing (like update_environment()) because the caller might pass a
@@ -544,17 +536,40 @@ HOOK_FUNC_DEF3(int, execve, char const *, filename, char * const *, argv, char *
         init_from_environment();
     }
 
-    char fds_env[strlen(ENV_NAME_FDS) + 1 + update_environment_buffer_size()];
-    strcpy(fds_env, ENV_NAME_FDS "=");
-    update_environment_buffer(fds_env + strlen(ENV_NAME_FDS) + 1);
+    char fds_env[strlen(ENV_NAME_PRIVATE_FDS)
+                 + 1 + update_environment_buffer_size()];
+    strcpy(fds_env, ENV_NAME_PRIVATE_FDS "=");
+    update_environment_buffer(fds_env + strlen(ENV_NAME_PRIVATE_FDS) + 1);
 
-    if (found) {
-        env_copy[index] = fds_env;
-    } else {
-        /* If the process removed ENV_NAME_FDS from the environment, re-add
-         * it. */
-        env_copy[count-1] = fds_env;
-        env_copy[count] = NULL;
+    int found = 0;
+    char **x_copy = env_copy;
+
+    /* Copy the environment manually; allows skipping elements. */
+    x = env;
+    while ((*x_copy = *x)) {
+        /* Remove ENV_NAME_FDS if we've already used its value. The new
+         * program must use the updated list from ENV_NAME_PRIVATE_FDS. */
+        if (used_fds_set_by_user
+                && !strncmp(*x, ENV_NAME_FDS "=", strlen(ENV_NAME_FDS) + 1)) {
+            x++;
+            continue;
+        /* Update ENV_NAME_PRIVATE_FDS. */
+        } else if (!strncmp(*x, ENV_NAME_PRIVATE_FDS "=",
+                            strlen(ENV_NAME_PRIVATE_FDS) + 1)) {
+            *x_copy = fds_env;
+            found = 1;
+        }
+
+        x++;
+        x_copy++;
+    }
+    /* The loop "condition" NULL-terminates env_copy. */
+
+    if (!found) {
+        /* If the process removed ENV_NAME_PRIVATE_FDS from the environment,
+         * re-add it. */
+        *x_copy++ = fds_env;
+        *x_copy++ = NULL;
     }
 
     return real_execve(filename, argv, env_copy);

src/constants.h

@@ -25,6 +25,7 @@
 #define ENV_NAME_PRE_STRING  "COLORED_STDERR_PRE"
 #define ENV_NAME_POST_STRING "COLORED_STDERR_POST"
 #define ENV_NAME_FORCE_WRITE "COLORED_STDERR_FORCE_WRITE"
+#define ENV_NAME_PRIVATE_FDS "COLORED_STDERR_PRIVATE_FDS"
 
 /* Strings written before/after each matched function. */
 #define DEFAULT_PRE_STRING  "\033[91m"

src/trackfds.h

@@ -72,8 +72,9 @@ static int init_tracked_fds_list(size_t count) {
 /* Load tracked file descriptors from the environment. The environment is used
  * to pass the information to child processes.
  *
- * ENV_NAME_FDS has the following format: Each descriptor as string followed
- * by a comma; there's a trailing comma. Example: "2,4,". */
+ * ENV_NAME_FDS and ENV_NAME_PRIVATE_FDS have the following format: Each
+ * descriptor as string followed by a comma; there's a trailing comma.
+ * Example: "2,4,". */
 static void init_from_environment(void) {
 #ifdef DEBUG
     debug("init_from_environment()\t\t[%d]\n", getpid());
@@ -92,14 +93,23 @@ static void init_from_environment(void) {
         force_write_to_non_tty = 1;
     }
 
+    /* Prefer user defined list of file descriptors, fall back to file
+     * descriptors passed through the environment from the parent process. */
     env = getenv(ENV_NAME_FDS);
+    if (env) {
+        used_fds_set_by_user = 1;
+    } else {
+        env = getenv(ENV_NAME_PRIVATE_FDS);
+    }
     if (!env) {
         errno = saved_errno;
         return;
     }
 #ifdef DEBUG
     debug("  getenv(\"%s\"): \"%s\"\n", ENV_NAME_FDS, env);
+    debug("  getenv(\"%s\"): \"%s\"\n", ENV_NAME_PRIVATE_FDS, env);
 #endif
+
     /* Environment must be treated read-only. */
     char env_copy[strlen(env) + 1];
     strcpy(env_copy, env);
@@ -225,16 +235,29 @@ static void update_environment(void) {
         return;
     }
 
+    int saved_errno = errno;
+
     char env[update_environment_buffer_size()];
     env[0] = 0;
 
     update_environment_buffer(env);
 
 #if 0
-    debug("    setenv(\"%s\", \"%s\", 1)\n", ENV_NAME_FDS, env);
+    debug("    setenv(\"%s\", \"%s\", 1)\n", ENV_NAME_PRIVATE_FDS, env);
 #endif
+    setenv(ENV_NAME_PRIVATE_FDS, env, 1 /* overwrite */);
 
-    setenv(ENV_NAME_FDS, env, 1 /* overwrite */);
+    /* Child processes must use ENV_NAME_PRIVATE_FDS to get the updated list
+     * of tracked file descriptors, not the static list provided by the user
+     * in ENV_NAME_FDS.
+     *
+     * But only remove it if the static list in ENV_NAME_FDS was loaded by
+     * init_from_environment() and merged into ENV_NAME_PRIVATE_FDS. */
+    if (used_fds_set_by_user) {
+        unsetenv(ENV_NAME_FDS);
+    }
+
+    errno = saved_errno;
 }