Re: pg_upgrade segfaults when given an invalid PGSERVICE value - Mailing list pgsql-hackers

On 13-03-18 09:17 PM, Bruce Momjian wrote:
> 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 treat the 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?

What other choices do we have? I don't think PQconndefaults() should 
continue on as if PGSERVICE wasn't set in the environment after a 
failure from parseServiceInfo.


> *  Should we document this?

Yes the documentation should indicate that PQconndefaults() can return 
NULL for more than just memory failures.

> *  Should we change this to just throw a warning?

How would we communicate warnings from PQconndefaults() back to the caller?


>
> Also, it seems pg_upgrade isn't the only utility that is confused:
>
>     contrib/postgres_fdw/option.c and contrib/dblink/dblink.c think
>     PQconndefaults() returning NULL means out of memory and report that
>     as the error string.
>     
>     bin/scripts/pg_isready.c and contrib/pg_upgrade/server.c have no
>     check for NULL return.
>     
>     libpq/test/uri-regress.c knows to throw a generic error message.
>
> So, we have some decisions and work to do.
>