Avoid putenv() problems by switching to setenv() (#2262)

Developers work around putenv() design flaws by writing more complex
code that still leaks memory (e.g., commit b1b2793 and commit 96b9d96).
We should follow putenv(3) manual page advice and use setenv() instead:

    The setenv() function is strongly preferred to putenv().

Kerberos builds have been using setenv() since 2009 commit 9ca29d2.
Twenty years ago, setenv() code failed to build on Solaris (see 2005
commit cff61cb), but modern Solaris does have setenv(3).

Since setenv(3) is a standard C library _extension_ unavailable on
Windows, we provide a setenv(3) replacement for MS Windows builds. Our
replacement should work correctly in known current use cases, but acts
differently if given a variable with an empty value. This replacement
does not make things worse because the old macro trick had the same
flaw. We do not know of an easy way to support empty variable values on
Windows the way setenv(3) does.

Also fixed negotiate_kerberos_auth undefined behavior caused by freeing
memory returned by getenv() when the helper was run without `-k keytab`.

diff --git a/compat/os/mswindows.h b/compat/os/mswindows.h
index 6e13a48645c2f078fb6249cc0a809fa40e3ba526..96a0faa9ba9f84522d347a51dde98e07af5ae487 100644 (file)
--- a/compat/os/mswindows.h
+++ b/compat/os/mswindows.h
@@ -107,7 +107,21 @@ SQUIDCEXTERN int WIN32_truncate(const char *pathname, off_t length);
 #define mkdir(p,F) mkdir((p))
 #define pclose _pclose
 #define popen _popen
-#define putenv _putenv
+
+inline int
+setenv(const char * const name, const char * const value, const int overwrite)
+{
+    if (!overwrite && getenv(name))
+        return 0;
+
+    // overwrite requested or the variable is not set
+
+    // XXX: Unlike POSIX.1 setenv(3) we want to emulate here, _putenv_s() treats
+    // `value` that points to an empty string specially: It removes the named
+    // variable (if any) and does not create a new variable with an empty value.
+    return (_putenv_s(name, value) == 0 ? 0 : -1);
+}
+
 #define setmode _setmode
 #define sleep(t) Sleep((t)*1000)
 #define umask _umask

diff --git a/configure.ac b/configure.ac
index 4f595eb9d43bd08d90d71475c688fd4f648babf5..12a984c682f4b8815618f0a4aef5e2cde156ccd5 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -2166,7 +2166,6 @@ AC_CHECK_FUNCS(\
        pthread_attr_setscope \
        pthread_setschedparam \
        pthread_sigmask \
-       putenv \
        regcomp \
        regexec \
        regfree \

diff --git a/src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc b/src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc
index 262f3498098666a3567cfdef1941379e69072fcc..acce0b0ee289066e84fc877c1fc830b3b5f038e8 100644 (file)
--- a/src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc
+++ b/src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc
@@ -348,12 +348,9 @@ main(int argc, char *const argv[])
     char default_keytab[MAXPATHLEN] = {};
 #if HAVE_KRB5_MEMORY_KEYTAB
     char *memory_keytab_name = nullptr;
-    char *memory_keytab_name_env = nullptr;
 #endif
     char *rcache_type = nullptr;
-    char *rcache_type_env = nullptr;
     char *rcache_dir = nullptr;
-    char *rcache_dir_env = nullptr;
     OM_uint32 major_status, minor_status;
     gss_ctx_id_t gss_context = GSS_C_NO_CONTEXT;
     gss_name_t client_name = GSS_C_NO_NAME;
@@ -507,28 +504,19 @@ main(int argc, char *const argv[])
     }
 
     if (rcache_type) {
-        rcache_type_env = (char *) xmalloc(strlen("KRB5RCACHETYPE=")+strlen(rcache_type)+1);
-        strcpy(rcache_type_env, "KRB5RCACHETYPE=");
-        strcat(rcache_type_env, rcache_type);
-        putenv(rcache_type_env);
+        (void)setenv("KRB5RCACHETYPE", rcache_type, 1);
         debug((char *) "%s| %s: INFO: Setting replay cache type to %s\n",
               LogTime(), PROGRAM, rcache_type);
     }
 
     if (rcache_dir) {
-        rcache_dir_env = (char *) xmalloc(strlen("KRB5RCACHEDIR=")+strlen(rcache_dir)+1);
-        strcpy(rcache_dir_env, "KRB5RCACHEDIR=");
-        strcat(rcache_dir_env, rcache_dir);
-        putenv(rcache_dir_env);
+        (void)setenv("KRB5RCACHEDIR", rcache_dir, 1);
         debug((char *) "%s| %s: INFO: Setting replay cache directory to %s\n",
               LogTime(), PROGRAM, rcache_dir);
     }
 
     if (keytab_name) {
-        keytab_name_env = (char *) xmalloc(strlen("KRB5_KTNAME=")+strlen(keytab_name)+1);
-        strcpy(keytab_name_env, "KRB5_KTNAME=");
-        strcat(keytab_name_env, keytab_name);
-        putenv(keytab_name_env);
+        (void)setenv("KRB5_KTNAME", keytab_name, 1);
     } else {
         keytab_name_env = getenv("KRB5_KTNAME");
         if (!keytab_name_env) {
@@ -558,10 +546,7 @@ main(int argc, char *const argv[])
                 debug((char *) "%s| %s: ERROR: Writing list into keytab %s\n",
                       LogTime(), PROGRAM, memory_keytab_name);
             } else {
-                memory_keytab_name_env = (char *) xmalloc(strlen("KRB5_KTNAME=")+strlen(memory_keytab_name)+1);
-                strcpy(memory_keytab_name_env, "KRB5_KTNAME=");
-                strcat(memory_keytab_name_env, memory_keytab_name);
-                putenv(memory_keytab_name_env);
+                (void)setenv("KRB5_KTNAME", memory_keytab_name, 1);
                 xfree(keytab_name);
                 keytab_name = xstrdup(memory_keytab_name);
                 debug((char *) "%s| %s: INFO: Changed keytab to %s\n",
@@ -639,15 +624,11 @@ main(int argc, char *const argv[])
             }
             xfree(token);
             xfree(rcache_type);
-            xfree(rcache_type_env);
             xfree(rcache_dir);
-            xfree(rcache_dir_env);
             xfree(keytab_name);
-            xfree(keytab_name_env);
 #if HAVE_KRB5_MEMORY_KEYTAB
             krb5_kt_close(context, memory_keytab);
             xfree(memory_keytab_name);
-            xfree(memory_keytab_name_env);
 #endif
             xfree(rfc_user);
             fprintf(stdout, "BH quit command\n");

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 412a14e95256cb5c30f1bf1a5664677c01723eb2..c0af0b6d14d7314287c42b2436da18dc08f6cea0 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -919,19 +919,8 @@ configDoConfigure(void)
 
             Config2.effectiveGroupID = pwd->pw_gid;
 
-#if HAVE_PUTENV
-            if (pwd->pw_dir && *pwd->pw_dir) {
-                // putenv() leaks by design; avoid leaks when nothing changes
-                static SBuf lastDir;
-                if (lastDir.isEmpty() || lastDir.cmp(pwd->pw_dir) != 0) {
-                    lastDir = pwd->pw_dir;
-                    int len = strlen(pwd->pw_dir) + 6;
-                    char *env_str = (char *)xcalloc(len, 1);
-                    snprintf(env_str, len, "HOME=%s", pwd->pw_dir);
-                    putenv(env_str);
-                }
-            }
-#endif
+            if (pwd->pw_dir && *pwd->pw_dir)
+                (void)setenv("HOME", pwd->pw_dir, 1);
         }
     } else {
         Config2.effectiveUserID = geteuid();

diff --git a/src/ipc.cc b/src/ipc.cc
index ec462cae3b59e5eacb96f04e9121b39978a677e5..11b22b138a62ea9902e43f27e6cb978a3be5990b 100644 (file)
--- a/src/ipc.cc
+++ b/src/ipc.cc
@@ -56,13 +56,7 @@ ipcCloseAllFD(int prfd, int pwfd, int crfd, int cwfd)
 static void
 PutEnvironment()
 {
-#if HAVE_PUTENV
-    char *env_str;
-    int tmp_s;
-    env_str = (char *)xcalloc((tmp_s = strlen(Debug::debugOptions) + 32), 1);
-    snprintf(env_str, tmp_s, "SQUID_DEBUG=%s", Debug::debugOptions);
-    putenv(env_str);
-#endif
+    (void)setenv("SQUID_DEBUG", Debug::debugOptions, 1);
 }
 
 pid_t

diff --git a/src/ipc_win32.cc b/src/ipc_win32.cc
index 7d64e65fdb84b898f96f90c95f9f8fae4c5e208b..64736ad4a535fd16d0f4ad67687d49f5d9818cd9 100644 (file)
--- a/src/ipc_win32.cc
+++ b/src/ipc_win32.cc
@@ -84,13 +84,7 @@ ipcCloseAllFD(int prfd, int pwfd, int crfd, int cwfd)
 static void
 PutEnvironment()
 {
-#if HAVE_PUTENV
-    char *env_str;
-    int tmp_s;
-    env_str = (char *)xcalloc((tmp_s = strlen(Debug::debugOptions) + 32), 1);
-    snprintf(env_str, tmp_s, "SQUID_DEBUG=%s", Debug::debugOptions);
-    putenv(env_str);
-#endif
+    (void)setenv("SQUID_DEBUG", Debug::debugOptions, 1);
 }
 
 pid_t