Re: pg_upgrade segfaults when given an invalid PGSERVICE value

On Mon, Mar 18, 2013 at 12:08:09PM -0400, Steve Singer wrote:
> If you try running pg_upgrade with the PGSERVICE environment
> variable set to some invalid/non-existent service pg_upgrade
> segfaults
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x000000000040bdd1 in check_pghost_envvar () at server.c:304
> 304        for (option = start; option->keyword != NULL; option++)
> (gdb) p start
> $5 = (PQconninfoOption *) 0x0
> 
> 
> PQconndefaults can return NULL if it has issues.
> 
> The attached patch prints a minimally useful error message. I don't
> a good way of getting a more useful error message out of
> PQconndefaults()
> 
> I checked this against master but it was reported to me as a issue in 9.2

Well, that's interesting.  There is no mention of PQconndefaults()
returning NULL except for out of memory:
      Returns a connection options array.  This can be used to determine      all possible
<function>PQconnectdb</function>options and their      current default values.  The return value points to an array of
   <structname>PQconninfoOption</structname> structures, which ends      with an entry having a null
<structfield>keyword</>pointer.  The
 
-->    null pointer is returned if memory could not be allocated. Note that      the current default values
(<structfield>val</structfield>fields)      will depend on environment variables and other context.  Callers      must
treatthe connection options data as read-only.
 

Looking at libpq/fe-connect.c::PQconndefaults(), it calls
conninfo_add_defaults(), which has this:
   /*    * If there's a service spec, use it to obtain any not-explicitly-given    * parameters.    */   if
(parseServiceInfo(options,errorMessage) != 0)       return false;
 

so it is clearly possible for PQconndefaults() to return NULL for
service file failures.  The questions are:

*  Is this what we want?
*  Should we document this?
*  Should we change this to just throw a warning?

Also, it seems pg_upgrade isn't the only utility that is confused:
contrib/postgres_fdw/option.c and contrib/dblink/dblink.c thinkPQconndefaults() returning NULL means out of memory and
reportthatas the error string.bin/scripts/pg_isready.c and contrib/pg_upgrade/server.c have nocheck for NULL
return.libpq/test/uri-regress.cknows to throw a generic error message.
 

So, we have some decisions and work to do.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +