Back in 2001 we decided to prefer putenv() over setenv() because the latter wasn't in POSIX (SUSv2) at the time. That decision has been overtaken by events: more recent editions of POSIX not only provide setenv() but recommend it over putenv(). So I think it's time to change our policy and prefer setenv(). We've had previous discussions about that but nobody did the legwork yet.
The attached patch provides the infrastructure to allow using setenv(). I added a src/port/ implementation of setenv() for any machines that haven't caught up with POSIX lately. (I've tested that code on my old dinosaur gaur, and I would not be surprised if that is the only machine anywhere that ever runs it. But I could be wrong.) I also extended win32env.c to support setenv(). I haven't made any serious effort to expunge all uses of putenv() in this version of the patch; I just wanted to exercise setenv() in both backend and frontend. Seeing that POSIX considers putenv() to be semi-deprecated, maybe we should try to eliminate all calls outside the (un)setenv implementations, but first it'd be good to see if win32env.c works. I also changed our unsetenv() emulations to make them return an int error indicator, as per POSIX. I have no desire to change the call sites to check for errors, but it seemed like our compatibility stubs should be compatible with the spec. (Note: I think that unsetenv() did return void in old BSD versions, before POSIX standardized it. So that might be a real reason not to change the callers.) regards, tom lane
diff --git a/configure b/configure index 11a4284e5b..07529825d1 100755 --- a/configure +++ b/configure @@ -15959,12 +15959,29 @@ case $host_os in # Windows uses a specialised env handler mingw*) +$as_echo "#define HAVE_SETENV 1" >>confdefs.h + + $as_echo "#define HAVE_UNSETENV 1" >>confdefs.h + ac_cv_func_setenv=yes ac_cv_func_unsetenv=yes ;; *) - ac_fn_c_check_func "$LINENO" "unsetenv" "ac_cv_func_unsetenv" + ac_fn_c_check_func "$LINENO" "setenv" "ac_cv_func_setenv" +if test "x$ac_cv_func_setenv" = xyes; then : + $as_echo "#define HAVE_SETENV 1" >>confdefs.h + +else + case " $LIBOBJS " in + *" setenv.$ac_objext "* ) ;; + *) LIBOBJS="$LIBOBJS setenv.$ac_objext" + ;; +esac + +fi + +ac_fn_c_check_func "$LINENO" "unsetenv" "ac_cv_func_unsetenv" if test "x$ac_cv_func_unsetenv" = xyes; then : $as_echo "#define HAVE_UNSETENV 1" >>confdefs.h diff --git a/configure.ac b/configure.ac index fc523c6aeb..7f855783f4 100644 --- a/configure.ac +++ b/configure.ac @@ -1757,11 +1757,13 @@ fi case $host_os in # Windows uses a specialised env handler mingw*) + AC_DEFINE(HAVE_SETENV, 1, [Define to 1 because replacement version used.]) AC_DEFINE(HAVE_UNSETENV, 1, [Define to 1 because replacement version used.]) + ac_cv_func_setenv=yes ac_cv_func_unsetenv=yes ;; *) - AC_REPLACE_FUNCS([unsetenv]) + AC_REPLACE_FUNCS([setenv unsetenv]) ;; esac diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 515ae95fe1..64d6e85555 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -1059,24 +1059,16 @@ pg_GSS_recvauth(Port *port) /* * Set default Kerberos keytab file for the Krb5 mechanism. * - * setenv("KRB5_KTNAME", pg_krb_server_keyfile, 0); except setenv() - * not always available. + * We don't overwrite any existing environment variable ... is that + * really a sane choice? */ - if (getenv("KRB5_KTNAME") == NULL) + if (setenv("KRB5_KTNAME", pg_krb_server_keyfile, 0) != 0) { - size_t kt_len = strlen(pg_krb_server_keyfile) + 14; - char *kt_path = malloc(kt_len); - - if (!kt_path || - snprintf(kt_path, kt_len, "KRB5_KTNAME=%s", - pg_krb_server_keyfile) != kt_len - 2 || - putenv(kt_path) != 0) - { - ereport(LOG, - (errcode(ERRCODE_OUT_OF_MEMORY), - errmsg("out of memory"))); - return STATUS_ERROR; - } + /* We assume the error must be "out of memory" */ + ereport(LOG, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); + return STATUS_ERROR; } } diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index c39d67645c..148a0984ea 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -105,20 +105,6 @@ char *localized_full_months[12 + 1]; static bool CurrentLocaleConvValid = false; static bool CurrentLCTimeValid = false; -/* Environment variable storage area */ - -#define LC_ENV_BUFSIZE (NAMEDATALEN + 20) - -static char lc_collate_envbuf[LC_ENV_BUFSIZE]; -static char lc_ctype_envbuf[LC_ENV_BUFSIZE]; - -#ifdef LC_MESSAGES -static char lc_messages_envbuf[LC_ENV_BUFSIZE]; -#endif -static char lc_monetary_envbuf[LC_ENV_BUFSIZE]; -static char lc_numeric_envbuf[LC_ENV_BUFSIZE]; -static char lc_time_envbuf[LC_ENV_BUFSIZE]; - /* Cache for collation-related knowledge */ typedef struct @@ -163,7 +149,6 @@ pg_perm_setlocale(int category, const char *locale) { char *result; const char *envvar; - char *envbuf; #ifndef WIN32 result = setlocale(category, locale); @@ -199,7 +184,7 @@ pg_perm_setlocale(int category, const char *locale) */ if (category == LC_CTYPE) { - static char save_lc_ctype[LC_ENV_BUFSIZE]; + static char save_lc_ctype[NAMEDATALEN + 20]; /* copy setlocale() return value before callee invokes it again */ strlcpy(save_lc_ctype, result, sizeof(save_lc_ctype)); @@ -216,16 +201,13 @@ pg_perm_setlocale(int category, const char *locale) { case LC_COLLATE: envvar = "LC_COLLATE"; - envbuf = lc_collate_envbuf; break; case LC_CTYPE: envvar = "LC_CTYPE"; - envbuf = lc_ctype_envbuf; break; #ifdef LC_MESSAGES case LC_MESSAGES: envvar = "LC_MESSAGES"; - envbuf = lc_messages_envbuf; #ifdef WIN32 result = IsoLocaleName(locale); if (result == NULL) @@ -236,26 +218,19 @@ pg_perm_setlocale(int category, const char *locale) #endif /* LC_MESSAGES */ case LC_MONETARY: envvar = "LC_MONETARY"; - envbuf = lc_monetary_envbuf; break; case LC_NUMERIC: envvar = "LC_NUMERIC"; - envbuf = lc_numeric_envbuf; break; case LC_TIME: envvar = "LC_TIME"; - envbuf = lc_time_envbuf; break; default: elog(FATAL, "unrecognized LC category: %d", category); - envvar = NULL; /* keep compiler quiet */ - envbuf = NULL; - return NULL; + return NULL; /* keep compiler quiet */ } - snprintf(envbuf, LC_ENV_BUFSIZE - 1, "%s=%s", envvar, result); - - if (putenv(envbuf)) + if (setenv(envvar, result, 1)) return NULL; return result; diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index f994c4216b..523d55dc65 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2355,8 +2355,7 @@ check_need_password(const char *authmethodlocal, const char *authmethodhost) void setup_pgdata(void) { - char *pgdata_get_env, - *pgdata_set_env; + char *pgdata_get_env; if (!pg_data) { @@ -2386,8 +2385,11 @@ setup_pgdata(void) * need quotes otherwise on Windows because paths there are most likely to * have embedded spaces. */ - pgdata_set_env = psprintf("PGDATA=%s", pg_data); - putenv(pgdata_set_env); + if (setenv("PGDATA", pg_data, 1) != 0) + { + pg_log_error("setenv failed"); + exit(1); + } } diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index de8f838e53..ddaa9e8e18 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -473,6 +473,9 @@ /* Define to 1 if you have the <security/pam_appl.h> header file. */ #undef HAVE_SECURITY_PAM_APPL_H +/* Define to 1 if you have the `setenv' function. */ +#undef HAVE_SETENV + /* Define to 1 if you have the `setproctitle' function. */ #undef HAVE_SETPROCTITLE diff --git a/src/include/port.h b/src/include/port.h index 5dfb00b07c..c631185c8b 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -447,8 +447,12 @@ extern size_t strnlen(const char *str, size_t maxlen); extern long random(void); #endif +#ifndef HAVE_SETENV +extern int setenv(const char *name, const char *value, int overwrite); +#endif + #ifndef HAVE_UNSETENV -extern void unsetenv(const char *name); +extern int unsetenv(const char *name); #endif #ifndef HAVE_SRANDOM diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index 59c7f35e3d..2ffe056a0f 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -490,7 +490,12 @@ extern void _dosmaperr(unsigned long); /* in port/win32env.c */ extern int pgwin32_putenv(const char *); -extern void pgwin32_unsetenv(const char *); +extern int pgwin32_setenv(const char *name, const char *value, int overwrite); +extern int pgwin32_unsetenv(const char *name); + +#define putenv(x) pgwin32_putenv(x) +#define setenv(x,y,z) pgwin32_setenv(x,y,z) +#define unsetenv(x) pgwin32_unsetenv(x) /* in port/win32security.c */ extern int pgwin32_is_service(void); @@ -499,9 +504,6 @@ extern int pgwin32_is_admin(void); /* Windows security token manipulation (in src/common/exec.c) */ extern BOOL AddUserToTokenDacl(HANDLE hToken); -#define putenv(x) pgwin32_putenv(x) -#define unsetenv(x) pgwin32_unsetenv(x) - /* Things that exist in MinGW headers, but need to be added to MSVC */ #ifdef _MSC_VER diff --git a/src/port/setenv.c b/src/port/setenv.c new file mode 100644 index 0000000000..d8a5647fb0 --- /dev/null +++ b/src/port/setenv.c @@ -0,0 +1,48 @@ +/*------------------------------------------------------------------------- + * + * setenv.c + * setenv() emulation for machines without it + * + * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/port/setenv.c + * + *------------------------------------------------------------------------- + */ + +#include "c.h" + + +int +setenv(const char *name, const char *value, int overwrite) +{ + char *envstr; + + /* Error conditions, per POSIX */ + if (name == NULL || name[0] == '\0' || strchr(name, '=') != NULL || + value == NULL) + { + errno = EINVAL; + return -1; + } + + /* No work if variable exists and we're not to replace it */ + if (overwrite == 0 && getenv(name) != NULL) + return 0; + + /* + * Add or replace the value using putenv(). This will leak memory if the + * same variable is repeatedly redefined, but there's little we can do + * about that when sitting atop putenv(). + */ + envstr = (char *) malloc(strlen(name) + strlen(value) + 2); + if (!envstr) /* not much we can do if no memory */ + return -1; + + sprintf(envstr, "%s=%s", name, value); + + return putenv(envstr); +} diff --git a/src/port/unsetenv.c b/src/port/unsetenv.c index f2028c2f83..a5f19f8db3 100644 --- a/src/port/unsetenv.c +++ b/src/port/unsetenv.c @@ -16,13 +16,20 @@ #include "c.h" -void +int unsetenv(const char *name) { char *envstr; + /* Error conditions, per POSIX */ + if (name == NULL || name[0] == '\0' || strchr(name, '=') != NULL) + { + errno = EINVAL; + return -1; + } + if (getenv(name) == NULL) - return; /* no work */ + return 0; /* no work */ /* * The technique embodied here works if libc follows the Single Unix Spec @@ -40,11 +47,12 @@ unsetenv(const char *name) envstr = (char *) malloc(strlen(name) + 2); if (!envstr) /* not much we can do if no memory */ - return; + return -1; /* Override the existing setting by forcibly defining the var */ sprintf(envstr, "%s=", name); - putenv(envstr); + if (putenv(envstr)) + return -1; /* Now we can clobber the variable definition this way: */ strcpy(envstr, "="); @@ -53,5 +61,5 @@ unsetenv(const char *name) * This last putenv cleans up if we have multiple zero-length names as a * result of unsetting multiple things. */ - putenv(envstr); + return putenv(envstr); } diff --git a/src/port/win32env.c b/src/port/win32env.c index 177488cc67..f5bed67297 100644 --- a/src/port/win32env.c +++ b/src/port/win32env.c @@ -1,8 +1,10 @@ /*------------------------------------------------------------------------- * * win32env.c - * putenv() and unsetenv() for win32, which update both process environment - * and caches in (potentially multiple) C run-time library (CRT) versions. + * putenv(), setenv(), and unsetenv() for win32. + * + * These functions update both the process environment and caches in + * (potentially multiple) C run-time library (CRT) versions. * * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California @@ -16,6 +18,11 @@ #include "c.h" + +/* + * Note that unlike POSIX putenv(), this doesn't use the passed-in string + * as permanent storage. + */ int pgwin32_putenv(const char *envval) { @@ -64,7 +71,7 @@ pgwin32_putenv(const char *envval) } *cp = '\0'; cp++; - if (strlen(cp)) + if (*cp) { /* * Only call SetEnvironmentVariable() when we are adding a variable, @@ -110,16 +117,47 @@ pgwin32_putenv(const char *envval) return _putenv(envval); } -void +int +pgwin32_setenv(const char *name, const char *value, int overwrite) +{ + int res; + char *envstr; + + /* Error conditions, per POSIX */ + if (name == NULL || name[0] == '\0' || strchr(name, '=') != NULL || + value == NULL) + { + errno = EINVAL; + return -1; + } + + /* No work if variable exists and we're not to replace it */ + if (overwrite == 0 && getenv(name) != NULL) + return 0; + + envstr = (char *) malloc(strlen(name) + strlen(value) + 2); + if (!envstr) /* not much we can do if no memory */ + return -1; + + sprintf(envstr, "%s=%s", name, value); + + res = pgwin32_putenv(envstr); + free(envstr); + return res; +} + +int pgwin32_unsetenv(const char *name) { + int res; char *envbuf; envbuf = (char *) malloc(strlen(name) + 2); if (!envbuf) - return; + return -1; sprintf(envbuf, "%s=", name); - pgwin32_putenv(envbuf); + res = pgwin32_putenv(envbuf); free(envbuf); + return res; } diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 22d6abd367..95d4e826b1 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -348,6 +348,7 @@ sub GenerateFiles HAVE_RL_FILENAME_QUOTING_FUNCTION => undef, HAVE_RL_RESET_SCREEN_SIZE => undef, HAVE_SECURITY_PAM_APPL_H => undef, + HAVE_SETENV => undef, HAVE_SETPROCTITLE => undef, HAVE_SETPROCTITLE_FAST => undef, HAVE_SETSID => undef,