Re: Clang 3.3 Analyzer Results - Mailing list pgsql-hackers

Kevin Grittner <kgrittn@ymail.com> writes:
> If nobody objects, I'll fix that small memory leak in the
> regression test driver.� Hopefully someone more familiar with
> pg_basebackup will fix the double-free (and related problems
> mentioned by Tom) in streamutil.c.

Here's a less convoluted (IMHO) approach to the password management logic
in streamutil.c.  One thing I really didn't care for about the existing
coding is that the loop-for-password included all the rest of the
function, even though there's no intention to retry for any purpose except
collecting a password.  So I moved up the bottom of the loop.  For ease of
review, I've not reindented the code below the new loop bottom, but would
do so before committing.

Any objections to this version?

            regards, tom lane

diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 1dfb80f..c89fca9 100644
*** a/src/bin/pg_basebackup/streamutil.c
--- b/src/bin/pg_basebackup/streamutil.c
*************** GetConnection(void)
*** 40,47 ****
      int            i;
      const char **keywords;
      const char **values;
-     char       *password = NULL;
      const char *tmpparam;
      PQconninfoOption *conn_opts = NULL;
      PQconninfoOption *conn_opt;
      char       *err_msg = NULL;
--- 40,47 ----
      int            i;
      const char **keywords;
      const char **values;
      const char *tmpparam;
+     bool        need_password;
      PQconninfoOption *conn_opts = NULL;
      PQconninfoOption *conn_opt;
      char       *err_msg = NULL;
*************** GetConnection(void)
*** 114,140 ****
          i++;
      }

      while (true)
      {
!         if (password)
!             free(password);

          if (dbpassword)
          {
-             /*
-              * We've saved a password when a previous connection succeeded,
-              * meaning this is the call for a second session to the same
-              * database, so just forcibly reuse that password.
-              */
              keywords[i] = "password";
              values[i] = dbpassword;
-             dbgetpassword = -1; /* Don't try again if this fails */
          }
!         else if (dbgetpassword == 1)
          {
!             password = simple_prompt(_("Password: "), 100, false);
!             keywords[i] = "password";
!             values[i] = password;
          }

          tmpconn = PQconnectdbParams(keywords, values, true);
--- 114,143 ----
          i++;
      }

+     /* If -W was given, force prompt for password, but only the first time */
+     need_password = (dbgetpassword == 1 && dbpassword == NULL);
+
      while (true)
      {
!         /* Get a new password if appropriate */
!         if (need_password)
!         {
!             if (dbpassword)
!                 free(dbpassword);
!             dbpassword = simple_prompt(_("Password: "), 100, false);
!             need_password = false;
!         }

+         /* Use (or reuse, on a subsequent connection) password if we have it */
          if (dbpassword)
          {
              keywords[i] = "password";
              values[i] = dbpassword;
          }
!         else
          {
!             keywords[i] = NULL;
!             values[i] = NULL;
          }

          tmpconn = PQconnectdbParams(keywords, values, true);
*************** GetConnection(void)
*** 150,163 ****
              exit(1);
          }

          if (PQstatus(tmpconn) == CONNECTION_BAD &&
              PQconnectionNeedsPassword(tmpconn) &&
              dbgetpassword != -1)
          {
-             dbgetpassword = 1;    /* ask for password next time */
              PQfinish(tmpconn);
!             continue;
          }

          if (PQstatus(tmpconn) != CONNECTION_OK)
          {
--- 153,169 ----
              exit(1);
          }

+         /* If we need a password and -w wasn't given, loop back and get one */
          if (PQstatus(tmpconn) == CONNECTION_BAD &&
              PQconnectionNeedsPassword(tmpconn) &&
              dbgetpassword != -1)
          {
              PQfinish(tmpconn);
!             need_password = true;
          }
+         else
+             break;
+     }

          if (PQstatus(tmpconn) != CONNECTION_OK)
          {
*************** GetConnection(void)
*** 204,212 ****
              exit(1);
          }

-         /* Store the password for next run */
-         if (password)
-             dbpassword = password;
          return tmpconn;
-     }
  }
--- 210,214 ----