Let's start using setenv() - Mailing list pgsql-hackers

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,