Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory - Mailing list pgsql-hackers

I wrote:
> Meh.  I guess there's not much point in arguing with facts on the
> ground.  Anders' proposed behavior seems like the way to go then,
> at least so far as libpq is concerned.

So I pushed that, but while working on it I grew quite annoyed
at the messy API exhibited by src/port/thread.c, particularly
at how declaring its functions in port.h requires #including
<netdb.h> and <pwd.h> there.  That means those headers are
read by every compile in our tree, though only a tiny number
of modules actually need either.  So here are a couple of
follow-on patches to improve that situation.

For pqGethostbyname, there is no consumer other than
src/port/getaddrinfo.c, which makes it even sillier because that
file isn't even compiled on a large majority of platforms, making
pqGethostbyname dead code that we nonetheless build everywhere.
So 0001 attached just moves that function into getaddrinfo.c.

For pqGetpwuid, the best solution seemed to be to add a
less platform-dependent API layer, which I did in 0002
attached.  Perhaps someone would object to the API details
I chose, but by and large this seems like an improvement
that reduces the amount of code duplication in the callers.

Thoughts?

            regards, tom lane

diff --git a/src/include/port.h b/src/include/port.h
index 22ea292a6d..df81fa97c6 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -14,7 +14,6 @@
 #define PG_PORT_H

 #include <ctype.h>
-#include <netdb.h>
 #include <pwd.h>

 /*
@@ -485,12 +484,6 @@ extern int    pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
                        size_t buflen, struct passwd **result);
 #endif

-extern int    pqGethostbyname(const char *name,
-                            struct hostent *resultbuf,
-                            char *buffer, size_t buflen,
-                            struct hostent **result,
-                            int *herrno);
-
 extern void pg_qsort(void *base, size_t nel, size_t elsize,
                      int (*cmp) (const void *, const void *));
 extern int    pg_qsort_strcmp(const void *a, const void *b);
diff --git a/src/port/Makefile b/src/port/Makefile
index b3754d8940..bfe1feb0d4 100644
--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -84,6 +84,10 @@ libpgport.a: $(OBJS)
     rm -f $@
     $(AR) $(AROPT) $@ $^

+# getaddrinfo.o and getaddrinfo_shlib.o need PTHREAD_CFLAGS (but getaddrinfo_srv.o does not)
+getaddrinfo.o: CFLAGS+=$(PTHREAD_CFLAGS)
+getaddrinfo_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)
+
 # thread.o and thread_shlib.o need PTHREAD_CFLAGS (but thread_srv.o does not)
 thread.o: CFLAGS+=$(PTHREAD_CFLAGS)
 thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)
diff --git a/src/port/getaddrinfo.c b/src/port/getaddrinfo.c
index b0ca4c778e..3284c6eb52 100644
--- a/src/port/getaddrinfo.c
+++ b/src/port/getaddrinfo.c
@@ -34,6 +34,14 @@
 #include "port/pg_bswap.h"


+#ifdef FRONTEND
+static int    pqGethostbyname(const char *name,
+                            struct hostent *resultbuf,
+                            char *buffer, size_t buflen,
+                            struct hostent **result,
+                            int *herrno);
+#endif
+
 #ifdef WIN32
 /*
  * The native routines may or may not exist on the Windows platform we are on,
@@ -394,3 +402,39 @@ getnameinfo(const struct sockaddr *sa, int salen,

     return 0;
 }
+
+/*
+ * Wrapper around gethostbyname() or gethostbyname_r() to mimic
+ * POSIX gethostbyname_r() behaviour, if it is not available or required.
+ */
+#ifdef FRONTEND
+static int
+pqGethostbyname(const char *name,
+                struct hostent *resultbuf,
+                char *buffer, size_t buflen,
+                struct hostent **result,
+                int *herrno)
+{
+#if defined(ENABLE_THREAD_SAFETY) && defined(HAVE_GETHOSTBYNAME_R)
+
+    /*
+     * broken (well early POSIX draft) gethostbyname_r() which returns 'struct
+     * hostent *'
+     */
+    *result = gethostbyname_r(name, resultbuf, buffer, buflen, herrno);
+    return (*result == NULL) ? -1 : 0;
+#else
+
+    /* no gethostbyname_r(), just use gethostbyname() */
+    *result = gethostbyname(name);
+
+    if (*result != NULL)
+        *herrno = h_errno;
+
+    if (*result != NULL)
+        return 0;
+    else
+        return -1;
+#endif
+}
+#endif                            /* FRONTEND */
diff --git a/src/port/thread.c b/src/port/thread.c
index c1040d4e24..a4c1672c89 100644
--- a/src/port/thread.c
+++ b/src/port/thread.c
@@ -76,41 +76,3 @@ pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
 #endif
 }
 #endif
-
-/*
- * Wrapper around gethostbyname() or gethostbyname_r() to mimic
- * POSIX gethostbyname_r() behaviour, if it is not available or required.
- * This function is called _only_ by our getaddrinfo() portability function.
- */
-#ifndef HAVE_GETADDRINFO
-int
-pqGethostbyname(const char *name,
-                struct hostent *resultbuf,
-                char *buffer, size_t buflen,
-                struct hostent **result,
-                int *herrno)
-{
-#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(HAVE_GETHOSTBYNAME_R)
-
-    /*
-     * broken (well early POSIX draft) gethostbyname_r() which returns 'struct
-     * hostent *'
-     */
-    *result = gethostbyname_r(name, resultbuf, buffer, buflen, herrno);
-    return (*result == NULL) ? -1 : 0;
-#else
-
-    /* no gethostbyname_r(), just use gethostbyname() */
-    *result = gethostbyname(name);
-
-    if (*result != NULL)
-        *herrno = h_errno;
-
-    if (*result != NULL)
-        return 0;
-    else
-        return -1;
-#endif
-}
-
-#endif
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b0d6988aff..4d72a6a8c1 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -18,6 +18,7 @@
 #include <sys/param.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
+#include <pwd.h>
 #include <unistd.h>
 #ifdef HAVE_SYS_SELECT_H
 #include <sys/select.h>
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f210ccbde8..3503605a7d 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -10,6 +10,7 @@
 #include <ctype.h>
 #include <limits.h>
 #include <math.h>
+#include <pwd.h>
 #include <signal.h>
 #ifndef WIN32
 #include <unistd.h>                /* for write() */
diff --git a/src/include/port.h b/src/include/port.h
index df81fa97c6..3f1716188b 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -14,7 +14,6 @@
 #define PG_PORT_H

 #include <ctype.h>
-#include <pwd.h>

 /*
  * Windows has enough specialized port stuff that we push most of it off
@@ -478,10 +477,10 @@ extern char *dlerror(void);
 #define RTLD_GLOBAL 0
 #endif

-/* thread.h */
+/* thread.c */
 #ifndef WIN32
-extern int    pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
-                       size_t buflen, struct passwd **result);
+extern bool pg_get_user_name(uid_t user_id, char *buffer, size_t buflen);
+extern bool pg_get_user_home_dir(uid_t user_id, char *buffer, size_t buflen);
 #endif

 extern void pg_qsort(void *base, size_t nel, size_t elsize,
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 8d2e4e5db4..aa4534578a 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -35,7 +35,6 @@
 #ifndef  MAXHOSTNAMELEN
 #include <netdb.h>                /* for MAXHOSTNAMELEN on some */
 #endif
-#include <pwd.h>
 #endif

 #include "common/md5.h"
@@ -1091,14 +1090,17 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)


 /*
- * pg_fe_getauthname
+ * pg_fe_getusername
  *
- * Returns a pointer to malloc'd space containing whatever name the user
- * has authenticated to the system.  If there is an error, return NULL,
- * and append a suitable error message to *errorMessage if that's not NULL.
+ * Returns a pointer to malloc'd space containing the name of the
+ * specified user_id.  If there is an error, return NULL, and append
+ * a suitable error message to *errorMessage if that's not NULL.
+ *
+ * Caution: on Windows, the user_id argument is ignored, and we always
+ * fetch the current user's name.
  */
 char *
-pg_fe_getauthname(PQExpBuffer errorMessage)
+pg_fe_getusername(uid_t user_id, PQExpBuffer errorMessage)
 {
     char       *result = NULL;
     const char *name = NULL;
@@ -1108,17 +1110,13 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
     char        username[256 + 1];
     DWORD        namesize = sizeof(username);
 #else
-    uid_t        user_id = geteuid();
     char        pwdbuf[BUFSIZ];
-    struct passwd pwdstr;
-    struct passwd *pw = NULL;
-    int            pwerr;
 #endif

     /*
      * Some users are using configure --enable-thread-safety-force, so we
      * might as well do the locking within our library to protect
-     * pqGetpwuid(). In fact, application developers can use getpwuid() in
+     * getpwuid(). In fact, application developers can use getpwuid() in
      * their application if they use the locking call we provide, or install
      * their own locking function using PQregisterThreadLock().
      */
@@ -1132,21 +1130,10 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
                           libpq_gettext("user name lookup failure: error code %lu\n"),
                           GetLastError());
 #else
-    pwerr = pqGetpwuid(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
-    if (pw != NULL)
-        name = pw->pw_name;
+    if (pg_get_user_name(user_id, pwdbuf, sizeof(pwdbuf)))
+        name = pwdbuf;
     else if (errorMessage)
-    {
-        if (pwerr != 0)
-            appendPQExpBuffer(errorMessage,
-                              libpq_gettext("could not look up local user ID %d: %s\n"),
-                              (int) user_id,
-                              strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
-        else
-            appendPQExpBuffer(errorMessage,
-                              libpq_gettext("local user with ID %d does not exist\n"),
-                              (int) user_id);
-    }
+        appendPQExpBuffer(errorMessage, "%s\n", pwdbuf);
 #endif

     if (name)
@@ -1162,6 +1149,23 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
     return result;
 }

+/*
+ * pg_fe_getauthname
+ *
+ * Returns a pointer to malloc'd space containing whatever name the user
+ * has authenticated to the system.  If there is an error, return NULL,
+ * and append a suitable error message to *errorMessage if that's not NULL.
+ */
+char *
+pg_fe_getauthname(PQExpBuffer errorMessage)
+{
+#ifdef WIN32
+    return pg_fe_getusername(0, errorMessage);
+#else
+    return pg_fe_getusername(geteuid(), errorMessage);
+#endif
+}
+

 /*
  * PQencryptPassword -- exported routine to encrypt a password with MD5
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 16d5e1da0f..f22b3fe648 100644
--- a/src/interfaces/libpq/fe-auth.h
+++ b/src/interfaces/libpq/fe-auth.h
@@ -20,6 +20,7 @@

 /* Prototypes for functions in fe-auth.c */
 extern int    pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn);
+extern char *pg_fe_getusername(uid_t user_id, PQExpBuffer errorMessage);
 extern char *pg_fe_getauthname(PQExpBuffer errorMessage);

 /* Mechanisms in fe-auth-scram.c */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a12e0180fd..5fc16be849 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2813,10 +2813,7 @@ keep_going:                        /* We will come back to here until there is
                     IS_AF_UNIX(conn->raddr.addr.ss_family))
                 {
 #ifndef WIN32
-                    char        pwdbuf[BUFSIZ];
-                    struct passwd pass_buf;
-                    struct passwd *pass;
-                    int            passerr;
+                    char       *remote_username;
 #endif
                     uid_t        uid;
                     gid_t        gid;
@@ -2839,28 +2836,20 @@ keep_going:                        /* We will come back to here until there is
                     }

 #ifndef WIN32
-                    passerr = pqGetpwuid(uid, &pass_buf, pwdbuf, sizeof(pwdbuf), &pass);
-                    if (pass == NULL)
-                    {
-                        if (passerr != 0)
-                            appendPQExpBuffer(&conn->errorMessage,
-                                              libpq_gettext("could not look up local user ID %d: %s\n"),
-                                              (int) uid,
-                                              strerror_r(passerr, sebuf, sizeof(sebuf)));
-                        else
-                            appendPQExpBuffer(&conn->errorMessage,
-                                              libpq_gettext("local user with ID %d does not exist\n"),
-                                              (int) uid);
-                        goto error_return;
-                    }
+                    remote_username = pg_fe_getusername(uid,
+                                                        &conn->errorMessage);
+                    if (remote_username == NULL)
+                        goto error_return;    /* message already logged */

-                    if (strcmp(pass->pw_name, conn->requirepeer) != 0)
+                    if (strcmp(remote_username, conn->requirepeer) != 0)
                     {
                         appendPQExpBuffer(&conn->errorMessage,
                                           libpq_gettext("requirepeer specifies \"%s\", but actual peer user name is
\"%s\"\n"),
-                                          conn->requirepeer, pass->pw_name);
+                                          conn->requirepeer, remote_username);
+                        free(remote_username);
                         goto error_return;
                     }
+                    free(remote_username);
 #else                            /* WIN32 */
                     /* should have failed with ENOSYS above */
                     Assert(false);
@@ -7271,16 +7260,7 @@ pqGetHomeDirectory(char *buf, int bufsize)

     home = getenv("HOME");
     if (home == NULL || home[0] == '\0')
-    {
-        char        pwdbuf[BUFSIZ];
-        struct passwd pwdstr;
-        struct passwd *pwd = NULL;
-
-        (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
-        if (pwd == NULL)
-            return false;
-        home = pwd->pw_dir;
-    }
+        return pg_get_user_home_dir(geteuid(), buf, bufsize);
     strlcpy(buf, home, bufsize);
     return true;
 #else
diff --git a/src/port/path.c b/src/port/path.c
index 5ac26f4bcf..69bb8fe40b 100644
--- a/src/port/path.c
+++ b/src/port/path.c
@@ -815,16 +815,7 @@ get_home_path(char *ret_path)

     home = getenv("HOME");
     if (home == NULL || home[0] == '\0')
-    {
-        char        pwdbuf[BUFSIZ];
-        struct passwd pwdstr;
-        struct passwd *pwd = NULL;
-
-        (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
-        if (pwd == NULL)
-            return false;
-        home = pwd->pw_dir;
-    }
+        return pg_get_user_home_dir(geteuid(), ret_path, MAXPGPATH);
     strlcpy(ret_path, home, MAXPGPATH);
     return true;
 #else
diff --git a/src/port/thread.c b/src/port/thread.c
index a4c1672c89..e2a570ec40 100644
--- a/src/port/thread.c
+++ b/src/port/thread.c
@@ -49,6 +49,7 @@
  *    One thread-safe solution for gethostbyname() might be to use getaddrinfo().
  */

+#ifndef WIN32

 /*
  * Wrapper around getpwuid() or getpwuid_r() to mimic POSIX getpwuid_r()
@@ -60,8 +61,7 @@
  * error during lookup: returns an errno code, *result is NULL
  * (caller should *not* assume that the errno variable is set)
  */
-#ifndef WIN32
-int
+static int
 pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
            size_t buflen, struct passwd **result)
 {
@@ -75,4 +75,74 @@ pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
     return (*result == NULL) ? errno : 0;
 #endif
 }
-#endif
+
+/*
+ * pg_get_user_name - get the name of the user with the given ID
+ *
+ * On success, the user name is returned into the buffer (of size buflen),
+ * and "true" is returned.  On failure, a localized error message is
+ * returned into the buffer, and "false" is returned.
+ */
+bool
+pg_get_user_name(uid_t user_id, char *buffer, size_t buflen)
+{
+    char        pwdbuf[BUFSIZ];
+    struct passwd pwdstr;
+    struct passwd *pw = NULL;
+    int            pwerr;
+
+    pwerr = pqGetpwuid(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
+    if (pw != NULL)
+    {
+        strlcpy(buffer, pw->pw_name, buflen);
+        return true;
+    }
+    if (pwerr != 0)
+        snprintf(buffer, buflen,
+                 _("could not look up local user ID %d: %s"),
+                 (int) user_id,
+                 strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
+    else
+        snprintf(buffer, buflen,
+                 _("local user with ID %d does not exist"),
+                 (int) user_id);
+    return false;
+}
+
+/*
+ * pg_get_user_home_dir - get the home directory of the user with the given ID
+ *
+ * On success, the directory path is returned into the buffer (of size buflen),
+ * and "true" is returned.  On failure, a localized error message is
+ * returned into the buffer, and "false" is returned.
+ *
+ * Note that this does not incorporate the common behavior of checking
+ * $HOME first, since it's independent of which user_id is queried.
+ */
+bool
+pg_get_user_home_dir(uid_t user_id, char *buffer, size_t buflen)
+{
+    char        pwdbuf[BUFSIZ];
+    struct passwd pwdstr;
+    struct passwd *pw = NULL;
+    int            pwerr;
+
+    pwerr = pqGetpwuid(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
+    if (pw != NULL)
+    {
+        strlcpy(buffer, pw->pw_dir, buflen);
+        return true;
+    }
+    if (pwerr != 0)
+        snprintf(buffer, buflen,
+                 _("could not look up local user ID %d: %s"),
+                 (int) user_id,
+                 strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
+    else
+        snprintf(buffer, buflen,
+                 _("local user with ID %d does not exist"),
+                 (int) user_id);
+    return false;
+}
+
+#endif                            /* !WIN32 */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index deb62c1c7b..ec3546d0c0 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -106,7 +106,7 @@ sub mkvcbuild
       pread.c preadv.c pwrite.c pwritev.c pg_bitutils.c
       pg_strong_random.c pgcheckdir.c pgmkdirp.c pgsleep.c pgstrcasecmp.c
       pqsignal.c mkdtemp.c qsort.c qsort_arg.c bsearch_arg.c quotes.c system.c
-      strerror.c tar.c thread.c
+      strerror.c tar.c
       win32env.c win32error.c win32ntdll.c
       win32security.c win32setlocale.c win32stat.c);