From d732d6f240624b0001b182b8f7514f649e27a09c Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Fri, 3 Oct 2025 09:40:25 +0000 Subject: [PATCH] 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`. --- compat/os/mswindows.h | 16 ++++++++++- configure.ac | 1 - .../kerberos/negotiate_kerberos_auth.cc | 27 +++---------------- src/cache_cf.cc | 15 ++--------- src/ipc.cc | 8 +----- src/ipc_win32.cc | 8 +----- 6 files changed, 23 insertions(+), 52 deletions(-) diff --git a/compat/os/mswindows.h b/compat/os/mswindows.h index 6e13a48645..96a0faa9ba 100644 --- 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 4f595eb9d4..12a984c682 100644 --- 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 262f349809..acce0b0ee2 100644 --- 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 412a14e952..c0af0b6d14 100644 --- 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 ec462cae3b..11b22b138a 100644 --- 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 7d64e65fdb..64736ad4a5 100644 --- 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 -- 2.47.3