Re: libpq 9.4 requires /etc/passwd?

Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Jan  9, 2015 at 06:57:02PM -0500, Tom Lane wrote:
>> Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name
>> of the user.  Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned
>> failure of pg_fe_getauthname() into a hard connection failure, whereas
>> previously it was harmless as long as the caller provided a username.
>>
>> I wonder if we shouldn't just revert that commit in toto.  Yeah,
>> identifying an out-of-memory error might be useful, but this cure
>> seems a lot worse than the disease.  What's more, this coding reports
>> *any* pg_fe_getauthname failure as "out of memory", which is much worse
>> than useless.
>>
>> Alternatively, maybe don't try to resolve username this way until
>> we've found that the caller isn't providing any username.

> I have developed the attached patch which documents why the user name
> lookup might fail, and sets the failure string to "".  It preserves the
> memory allocation failure behavior.

I'm unimpressed with that patch.  It does nothing to fix the fundamental
problem, which is that error handling in this area is many bricks shy of
a load.  And to the extent that it addresses Christoph's complaint, it
does so only in a roundabout and utterly undocumented way.

I think we need to suck it up and fix pg_fe_getauthname to have proper
error reporting, which in turns means fixing pqGetpwuid (since the API
for that is incapable of distinguishing "no such user" from "error during
lookup").  Accordingly, I propose the attached patch instead.  Some
notes:

* According to Microsoft's documentation, Windows' GetUserName() does not
set errno (it would be mildly astonishing if it did...).  The existing
error-handling code in src/common/username.c is therefore wrong too.

* port.h seems to think it should declare pqGetpwuid() if __CYGWIN__,
but noplace else agrees with that.

* I made pg_fe_getauthname's Windows username[] array 256 bytes for
consistency with username.c, where some actual research seems to have
been expended on how large it ought to be.

* pqGetpwuid thinks there was once a four-argument spec for getpwuid_r.
That must have been in the late bronze age, because even in 1992's Single
Unix Spec it's five arguments.  And the SUS is our standard benchmark
for Unix portability.  So I think we could rip that out, along with the
corresponding configure test, at least in HEAD.  I've not done so here
though.

In principle, changing the API specs for pg_fe_getauthname and pqGetpwuid
should be safe because we don't officially export those functions.  In
practice, on platforms that don't honor the export restriction list it's
theoretically possible that somebody is calling those from third-party
code.  So what I propose we do with this is patch HEAD and 9.4 only.
We need to do *something* in 9.4 to address Christoph's complaint, and
that branch is new enough that we can probably get away with changing
officially-unsupported APIs.  The lack of other field complaints makes
me okay with not trying to fix these issues further back.

Comments?

            regards, tom lane

diff --git a/src/common/username.c b/src/common/username.c
index ee5ef1c..ac9a898 100644
*** a/src/common/username.c
--- b/src/common/username.c
*************** get_user_name(char **errstr)
*** 58,64 ****

      if (!GetUserName(username, &len))
      {
!         *errstr = psprintf(_("user name lookup failure: %s"), strerror(errno));
          return NULL;
      }

--- 58,65 ----

      if (!GetUserName(username, &len))
      {
!         *errstr = psprintf(_("user name lookup failure: error code %lu"),
!                            GetLastError());
          return NULL;
      }

diff --git a/src/include/port.h b/src/include/port.h
index 1d53e4e..26d7fcd 100644
*** a/src/include/port.h
--- b/src/include/port.h
*************** extern void srandom(unsigned int seed);
*** 433,439 ****
  /* thread.h */
  extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen);

! #if !defined(WIN32) || defined(__CYGWIN__)
  extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer,
             size_t buflen, struct passwd ** result);
  #endif
--- 433,439 ----
  /* thread.h */
  extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen);

! #ifndef WIN32
  extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer,
             size_t buflen, struct passwd ** result);
  #endif
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 179793e..5a6a1e4 100644
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_fe_sendauth(AuthRequest areq, PGconn
*** 714,735 ****
  /*
   * pg_fe_getauthname
   *
!  * Returns a pointer to dynamic space containing whatever name the user
!  * has authenticated to the system.  If there is an error, return NULL.
   */
  char *
! pg_fe_getauthname(void)
  {
      const char *name = NULL;
-     char       *authn;

  #ifdef WIN32
!     char        username[128];
      DWORD        namesize = sizeof(username) - 1;
  #else
      char        pwdbuf[BUFSIZ];
      struct passwd pwdstr;
      struct passwd *pw = NULL;
  #endif

      /*
--- 714,738 ----
  /*
   * 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 put a suitable error message in *errorMessage if that's not NULL.
   */
  char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
  {
+     char       *result = NULL;
      const char *name = NULL;

  #ifdef WIN32
!     char        username[256];
      DWORD        namesize = sizeof(username) - 1;
  #else
+     uid_t        user_id = geteuid();
      char        pwdbuf[BUFSIZ];
      struct passwd pwdstr;
      struct passwd *pw = NULL;
+     int            pwerr;
  #endif

      /*
*************** pg_fe_getauthname(void)
*** 741,764 ****
       */
      pglock_thread();

-     /*
-      * We document PQconndefaults() to return NULL for a memory allocation
-      * failure.  We don't have an API to return a user name lookup failure, so
-      * we just assume it always succeeds.
-      */
  #ifdef WIN32
      if (GetUserName(username, &namesize))
          name = username;
  #else
!     if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pw) == 0)
          name = pw->pw_name;
  #endif

!     authn = name ? strdup(name) : NULL;

      pgunlock_thread();

!     return authn;
  }


--- 744,785 ----
       */
      pglock_thread();

  #ifdef WIN32
      if (GetUserName(username, &namesize))
          name = username;
+     else if (errorMessage)
+         printfPQExpBuffer(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;
+     else if (errorMessage)
+     {
+         if (pwerr != 0)
+             printfPQExpBuffer(errorMessage,
+                    libpq_gettext("could not look up local user ID %d: %s\n"),
+                               (int) user_id,
+                               pqStrerror(pwerr, pwdbuf, sizeof(pwdbuf)));
+         else
+             printfPQExpBuffer(errorMessage,
+                      libpq_gettext("local user with ID %d does not exist\n"),
+                               (int) user_id);
+     }
  #endif

!     if (name)
!     {
!         result = strdup(name);
!         if (result == NULL && errorMessage)
!             printfPQExpBuffer(errorMessage,
!                               libpq_gettext("out of memory\n"));
!     }

      pgunlock_thread();

!     return result;
  }


diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 59b6c16..8d35767 100644
*** a/src/interfaces/libpq/fe-auth.h
--- b/src/interfaces/libpq/fe-auth.h
***************
*** 19,24 ****


  extern int    pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);

  #endif   /* FE_AUTH_H */
--- 19,24 ----


  extern int    pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(PQExpBuffer errorMessage);

  #endif   /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index b2f556c..25961b1 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** static bool
*** 765,774 ****
  connectOptions2(PGconn *conn)
  {
      /*
       * If database name was not given, default it to equal user name
       */
!     if ((conn->dbName == NULL || conn->dbName[0] == '\0')
!         && conn->pguser != NULL)
      {
          if (conn->dbName)
              free(conn->dbName);
--- 765,790 ----
  connectOptions2(PGconn *conn)
  {
      /*
+      * If user name was not given, fetch it.  (Most likely, the fetch will
+      * fail, since the only way we get here is if pg_fe_getauthname() failed
+      * during conninfo_add_defaults().  But now we want an error message.)
+      */
+     if (conn->pguser == NULL || conn->pguser[0] == '\0')
+     {
+         if (conn->pguser)
+             free(conn->pguser);
+         conn->pguser = pg_fe_getauthname(&conn->errorMessage);
+         if (!conn->pguser)
+         {
+             conn->status = CONNECTION_BAD;
+             return false;
+         }
+     }
+
+     /*
       * If database name was not given, default it to equal user name
       */
!     if (conn->dbName == NULL || conn->dbName[0] == '\0')
      {
          if (conn->dbName)
              free(conn->dbName);
*************** keep_going:                        /* We will come back to
*** 1967,1972 ****
--- 1983,1989 ----
                      char        pwdbuf[BUFSIZ];
                      struct passwd pass_buf;
                      struct passwd *pass;
+                     int            passerr;
                      uid_t        uid;
                      gid_t        gid;

*************** keep_going:                        /* We will come back to
*** 1987,1999 ****
                          goto error_return;
                      }

!                     pqGetpwuid(uid, &pass_buf, pwdbuf, sizeof(pwdbuf), &pass);
!
                      if (pass == NULL)
                      {
!                         appendPQExpBuffer(&conn->errorMessage,
!                                           libpq_gettext("local user with ID %d does not exist\n"),
!                                           (int) uid);
                          goto error_return;
                      }

--- 2004,2021 ----
                          goto error_return;
                      }

!                     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,
!                                               pqStrerror(passerr, sebuf, sizeof(sebuf)));
!                         else
!                             appendPQExpBuffer(&conn->errorMessage,
!                                               libpq_gettext("local user with ID %d does not exist\n"),
!                                               (int) uid);
                          goto error_return;
                      }

*************** conninfo_add_defaults(PQconninfoOption *
*** 4605,4622 ****
          }

          /*
!          * Special handling for "user" option
           */
          if (strcmp(option->keyword, "user") == 0)
          {
!             option->val = pg_fe_getauthname();
!             if (!option->val)
!             {
!                 if (errorMessage)
!                     printfPQExpBuffer(errorMessage,
!                                       libpq_gettext("out of memory\n"));
!                 return false;
!             }
              continue;
          }
      }
--- 4627,4641 ----
          }

          /*
!          * Special handling for "user" option.  Note that if pg_fe_getauthname
!          * fails, we just leave the value as NULL; there's no need for this to
!          * be an error condition if the caller provides a user name.  The only
!          * reason we do this now at all is so that callers of PQconndefaults
!          * will see a correct default (barring error, of course).
           */
          if (strcmp(option->keyword, "user") == 0)
          {
!             option->val = pg_fe_getauthname(NULL);
              continue;
          }
      }
*************** pqGetHomeDirectory(char *buf, int bufsiz
*** 5843,5849 ****
      struct passwd pwdstr;
      struct passwd *pwd = NULL;

!     if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) != 0)
          return false;
      strlcpy(buf, pwd->pw_dir, bufsize);
      return true;
--- 5862,5869 ----
      struct passwd pwdstr;
      struct passwd *pwd = NULL;

!     (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
!     if (pwd == NULL)
          return false;
      strlcpy(buf, pwd->pw_dir, bufsize);
      return true;
diff --git a/src/port/path.c b/src/port/path.c
index e8faac3..d0f72df 100644
*** a/src/port/path.c
--- b/src/port/path.c
*************** get_home_path(char *ret_path)
*** 777,783 ****
      struct passwd pwdstr;
      struct passwd *pwd = NULL;

!     if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) != 0)
          return false;
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
--- 777,784 ----
      struct passwd pwdstr;
      struct passwd *pwd = NULL;

!     (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
!     if (pwd == NULL)
          return false;
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
diff --git a/src/port/thread.c b/src/port/thread.c
index 1568803..aab7451 100644
*** a/src/port/thread.c
--- b/src/port/thread.c
*************** pqStrerror(int errnum, char *strerrbuf,
*** 83,88 ****
--- 83,94 ----
  /*
   * Wrapper around getpwuid() or getpwuid_r() to mimic POSIX getpwuid_r()
   * behaviour, if it is not available or required.
+  *
+  * Per POSIX, the possible cases are:
+  * success: returns zero, *result is non-NULL
+  * uid not found: returns zero, *result is NULL
+  * error during lookup: returns an errno code, *result is NULL
+  * (caller should *not* assume that the errno variable is set)
   */
  #ifndef WIN32
  int
*************** pqGetpwuid(uid_t uid, struct passwd * re
*** 93,114 ****

  #ifdef GETPWUID_R_5ARG
      /* POSIX version */
!     getpwuid_r(uid, resultbuf, buffer, buflen, result);
  #else

      /*
       * Early POSIX draft of getpwuid_r() returns 'struct passwd *'.
       * getpwuid_r(uid, resultbuf, buffer, buflen)
       */
      *result = getpwuid_r(uid, resultbuf, buffer, buflen);
  #endif
  #else
-
      /* no getpwuid_r() available, just use getpwuid() */
      *result = getpwuid(uid);
  #endif
-
-     return (*result == NULL) ? -1 : 0;
  }
  #endif

--- 99,123 ----

  #ifdef GETPWUID_R_5ARG
      /* POSIX version */
!     return getpwuid_r(uid, resultbuf, buffer, buflen, result);
  #else

      /*
       * Early POSIX draft of getpwuid_r() returns 'struct passwd *'.
       * getpwuid_r(uid, resultbuf, buffer, buflen)
       */
+     errno = 0;
      *result = getpwuid_r(uid, resultbuf, buffer, buflen);
+     /* paranoia: ensure we return zero on success */
+     return (*result == NULL) ? errno : 0;
  #endif
  #else
      /* no getpwuid_r() available, just use getpwuid() */
+     errno = 0;
      *result = getpwuid(uid);
+     /* paranoia: ensure we return zero on success */
+     return (*result == NULL) ? errno : 0;
  #endif
  }
  #endif