Thread: pg_upgrade segfaults when given an invalid PGSERVICE value
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 Steve
Attachment
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. +
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. >
On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote: > >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. True. Ignoring a service specification seems wrong, and issuing a warning message weak. > >* Should we document this? > > Yes the documentation should indicate that PQconndefaults() can > return NULL for more than just memory failures. The attached patch fixes this. I am unclear about backpatching this as it hits lot of code, is rare, and adds new translation strings. On the other hand, it does crash the applications. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote:
>>> * Should we document this?
>> Yes the documentation should indicate that PQconndefaults() can
>> return NULL for more than just memory failures.
> The attached patch fixes this. I am unclear about backpatching this as
> it hits lot of code, is rare, and adds new translation strings. On the
> other hand, it does crash the applications.
I don't particularly care for hard-wiring knowledge that bad service
name is the only other reason for PQconndefaults to fail (even assuming
that that's true, a point not in evidence ATM). I care even less for
continuing to use ERRCODE_FDW_OUT_OF_MEMORY for errors clearly outside
its meaning.
I think we should either change PQconndefaults to *not* fail in this
circumstance, or find a way to return an error message.
regards, tom lane
On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote: > >>> * Should we document this? > > >> Yes the documentation should indicate that PQconndefaults() can > >> return NULL for more than just memory failures. > > > The attached patch fixes this. I am unclear about backpatching this as > > it hits lot of code, is rare, and adds new translation strings. On the > > other hand, it does crash the applications. > > I don't particularly care for hard-wiring knowledge that bad service > name is the only other reason for PQconndefaults to fail (even assuming > that that's true, a point not in evidence ATM). I care even less for > continuing to use ERRCODE_FDW_OUT_OF_MEMORY for errors clearly outside > its meaning. Yes, overloading that error code was bad. > I think we should either change PQconndefaults to *not* fail in this > circumstance, or find a way to return an error message. Well, Steve Singer didn't like the idea of ignoring a service lookup failure. What do others think? We can throw a warning, but there is no way to know if the application allows the user to see it. Adding a way to communicate the service failure reason to the user would require a libpq API change, unless we go crazy and say -1 means service failure, and assume -1 can't be a valid pointer. Perhaps we need to remove the memory references and just report a failure, and mention services as a possible cause. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote:
>> I think we should either change PQconndefaults to *not* fail in this
>> circumstance, or find a way to return an error message.
> Well, Steve Singer didn't like the idea of ignoring a service lookup
> failure. What do others think? We can throw a warning, but there is no
> way to know if the application allows the user to see it.
Short of changing PQconndefaults's API, it seems like the only
reasonable answer is to not fail *in the context of PQconndefaults*.
We could still fail for bad service name in a real connection operation
(where there is an opportunity to return an error message).
While this surely isn't the nicest answer, it doesn't seem totally
unreasonable to me. A bad service name indeed does not contribute
anything to the set of defaults available.
regards, tom lane
On Wed, Mar 20, 2013 at 01:30:20PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote: > >> I think we should either change PQconndefaults to *not* fail in this > >> circumstance, or find a way to return an error message. > > > Well, Steve Singer didn't like the idea of ignoring a service lookup > > failure. What do others think? We can throw a warning, but there is no > > way to know if the application allows the user to see it. > > Short of changing PQconndefaults's API, it seems like the only > reasonable answer is to not fail *in the context of PQconndefaults*. > We could still fail for bad service name in a real connection operation > (where there is an opportunity to return an error message). Yes, that is _a_ plan. > While this surely isn't the nicest answer, it doesn't seem totally > unreasonable to me. A bad service name indeed does not contribute > anything to the set of defaults available. I think the concern is that the services file could easily change the defaults that are used for connecting, though you could argue that the real defaults for a bad service entry are properly returned. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 13-03-20 02:17 PM, Bruce Momjian wrote: > On Wed, Mar 20, 2013 at 01:30:20PM -0400, Tom Lane wrote: >> While this surely isn't the nicest answer, it doesn't seem totally >> unreasonable to me. A bad service name indeed does not contribute >> anything to the set of defaults available. > > I think the concern is that the services file could easily change the > defaults that are used for connecting, though you could argue that the > real defaults for a bad service entry are properly returned. Yes, my concern is that if I have a typo in the value of PGSERVICE I don't want to end up getting connected a connection to localhost instead of an error. From a end-user expectations point of view I am okay with somehow marking the structure returned by PQconndefaults in a way that the connect calls will later fail.
Steve Singer <ssinger@ca.afilias.info> writes:
> On 13-03-20 02:17 PM, Bruce Momjian wrote:
>> I think the concern is that the services file could easily change the
>> defaults that are used for connecting, though you could argue that the
>> real defaults for a bad service entry are properly returned.
> Yes, my concern is that if I have a typo in the value of PGSERVICE I
> don't want to end up getting connected a connection to localhost instead
> of an error.
> From a end-user expectations point of view I am okay with somehow
> marking the structure returned by PQconndefaults in a way that the
> connect calls will later fail.
Unless the program changes the value of PGSERVICE, surely all subsequent
connection attempts will fail for the same reason, regardless of what
PQconndefaults returns?
regards, tom lane
On 13-03-20 05:54 PM, Tom Lane wrote: > Steve Singer <ssinger@ca.afilias.info> writes: > >> From a end-user expectations point of view I am okay with somehow >> marking the structure returned by PQconndefaults in a way that the >> connect calls will later fail. > > Unless the program changes the value of PGSERVICE, surely all subsequent > connection attempts will fail for the same reason, regardless of what > PQconndefaults returns? > > regards, tom lane > > So your proposing we do something like the attached patch? Where we change conninfo_add_defaults to ignore an invalid PGSERVICE if being called by PQconndefaults() but keep the existing behaviour in other contexts where it is actually being used to establish a connection? In this case even if someone takes the result of PQconndefaults and uses that to build connection options for a new connection it should fail when it does the pgservice lookup when establishing the connection. That sounds reasonable to me. Steve
Attachment
On Mon, Mar 25, 2013 at 10:59:21AM -0400, Steve Singer wrote: > On 13-03-20 05:54 PM, Tom Lane wrote: > >Steve Singer <ssinger@ca.afilias.info> writes: > > > > >> From a end-user expectations point of view I am okay with somehow > >>marking the structure returned by PQconndefaults in a way that the > >>connect calls will later fail. > > > >Unless the program changes the value of PGSERVICE, surely all subsequent > >connection attempts will fail for the same reason, regardless of what > >PQconndefaults returns? > > > > regards, tom lane > > > > > > So your proposing we do something like the attached patch? Where we > change conninfo_add_defaults to ignore an invalid PGSERVICE if being > called by PQconndefaults() but keep the existing behaviour in other > contexts where it is actually being used to establish a connection? > > In this case even if someone takes the result of PQconndefaults and > uses that to build connection options for a new connection it should > fail when it does the pgservice lookup when establishing the > connection. That sounds reasonable to me. I was just about to look at this --- thanks for doing the legwork. Your fix is for conninfo_add_defaults() to conditionally fail, and that is a logical approach. One big problem I see is that effectively we have made conninfo_add_defaults() to conditionally fail based on a missing service (ignore_missing_service), and I think that reinforced my feeling that having PQconndefaults() not fail on a missing service is just an awkward approach to the problem. We are taking this approach because PQconndefaults() doesn't have an API to return the error cause, while other API calls do. Returning true so we can later report the right error from a later API call just feels wrong. However, I am also unclear about what alternative to suggest, except to sprinkle a "possible service problem" message to all the call failure. If we do want to the route of the patch, we should document this in the docs and C code so it is clear why we went in this direction. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> We are taking this approach because PQconndefaults() doesn't have an API
> to return the error cause, while other API calls do. Returning true so
> we can later report the right error from a later API call just feels
> wrong.
Well, plan B would be to invent a replacement function that does have
the ability to return an error message, but that seems like a lot of
work for a problem that's so marginal that it wasn't noticed till now.
(It's not so much creating the function that worries me, it's fixing
clients to use it.)
Plan C would be to redefine bogus value of PGSERVICE as not an error,
period.
regards, tom lane
On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > We are taking this approach because PQconndefaults() doesn't have an API > > to return the error cause, while other API calls do. Returning true so > > we can later report the right error from a later API call just feels > > wrong. > > Well, plan B would be to invent a replacement function that does have > the ability to return an error message, but that seems like a lot of > work for a problem that's so marginal that it wasn't noticed till now. > (It's not so much creating the function that worries me, it's fixing > clients to use it.) > > Plan C would be to redefine bogus value of PGSERVICE as not an error, > period. Given all of these poor options, is defining a PQconndefaults() as perhaps out of memory or a service file problem really not better? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote:
>> Well, plan B would be to invent a replacement function that does have
>> the ability to return an error message, but that seems like a lot of
>> work for a problem that's so marginal that it wasn't noticed till now.
>> (It's not so much creating the function that worries me, it's fixing
>> clients to use it.)
>>
>> Plan C would be to redefine bogus value of PGSERVICE as not an error,
>> period.
> Given all of these poor options, is defining a PQconndefaults() as
> perhaps out of memory or a service file problem really not better?
Uh ... no. In the first place, what evidence have you got that those
are (and will continue to be) the only two possible causes? In the
second place, this still requires changing every client of
PQconndefaults(), even if it's only to the extent of fixing their
error message texts. If we're going to do that, I'd rather ask them
to change to a more future-proof solution.
regards, tom lane
On 13-03-26 12:40 AM, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: >> On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote: >>> Well, plan B would be to invent a replacement function that does have >>> the ability to return an error message, but that seems like a lot of >>> work for a problem that's so marginal that it wasn't noticed till now. >>> (It's not so much creating the function that worries me, it's fixing >>> clients to use it.) >>> >>> Plan C would be to redefine bogus value of PGSERVICE as not an error, >>> period. > >> Given all of these poor options, is defining a PQconndefaults() as >> perhaps out of memory or a service file problem really not better? > > Uh ... no. In the first place, what evidence have you got that those > are (and will continue to be) the only two possible causes? In the > second place, this still requires changing every client of > PQconndefaults(), even if it's only to the extent of fixing their > error message texts. If we're going to do that, I'd rather ask them > to change to a more future-proof solution. > So to summarise: Plan A: The first patch I attached for pg_upgrade + documentation changes, and changing the other places that call PQconndefaults() to accept failures on either out of memory, or an invalid PGSERVICE Plan B: Create a new function PQconndefaults2(char * errorBuffer) or something similar that returned error information to the caller. Plan C: PQconndefaults() just ignores an invalid service but connection attempts fail because other callers of conninfo_add_defaults still pay attention to connection failures. This is the second patch I sent. Plan D: Service lookup failures are always ignored by conninfo_add_defaults. If you attempt to connect with a bad PGSERVICE set it will behave as if no PGSERVICE value was set. I don't think anyone explicitly proposed this yet. Plan 'D' is the only option that I'm opposed to, it will effect a lot more applications then ones that call PQconndefaults() and I feel it will confuse users. I'm not convinced plan B is worth the effort of having to maintain two versions of PQconndefaults() for a while to fix a corner case. > regards, tom lane > >
On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote: > So to summarise: > > Plan A: The first patch I attached for pg_upgrade + documentation > changes, and changing the other places that call PQconndefaults() to > accept failures on either out of memory, or an invalid PGSERVICE > > Plan B: Create a new function PQconndefaults2(char * errorBuffer) or > something similar that returned error information to the caller. > > Plan C: PQconndefaults() just ignores an invalid service but > connection attempts fail because other callers of > conninfo_add_defaults still pay attention to connection failures. > This is the second patch I sent. > > Plan D: Service lookup failures are always ignored by > conninfo_add_defaults. If you attempt to connect with a bad > PGSERVICE set it will behave as if no PGSERVICE value was set. I > don't think anyone explicitly proposed this yet. > > Plan 'D' is the only option that I'm opposed to, it will effect a > lot more applications then ones that call PQconndefaults() and I > feel it will confuse users. > > I'm not convinced plan B is worth the effort of having to maintain > two versions of PQconndefaults() for a while to fix a corner case. Yep, that's pretty much it. I agree B is overkill, and D seems dangerous. I think Tom likes C --- my only question is how many people will be confused that C returns inaccurate information, because though it returns connection options, it doesn't process the service file, while forced processing of the service file for real connections throws an error. We might be fine with C, but it must be documented in the C code and SGML docs. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote: > So to summarise: > > Plan A: The first patch I attached for pg_upgrade + documentation > changes, and changing the other places that call PQconndefaults() to > accept failures on either out of memory, or an invalid PGSERVICE > > Plan B: Create a new function PQconndefaults2(char * errorBuffer) or > something similar that returned error information to the caller. > > Plan C: PQconndefaults() just ignores an invalid service but > connection attempts fail because other callers of > conninfo_add_defaults still pay attention to connection failures. > This is the second patch I sent. > > Plan D: Service lookup failures are always ignored by > conninfo_add_defaults. If you attempt to connect with a bad > PGSERVICE set it will behave as if no PGSERVICE value was set. I > don't think anyone explicitly proposed this yet. > > Plan 'D' is the only option that I'm opposed to, it will effect a > lot more applications then ones that call PQconndefaults() and I > feel it will confuse users. > > I'm not convinced plan B is worth the effort of having to maintain > two versions of PQconndefaults() for a while to fix a corner case. OK, I am back to looking at this issue from March. I went with option C, patch attached, which seems to be the most popular. It is similar to Steve's patch, but structured differently and includes a doc patch. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On Fri, Nov 29, 2013 at 04:14:00PM -0500, Bruce Momjian wrote: > On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote: > > So to summarise: > > > > Plan A: The first patch I attached for pg_upgrade + documentation > > changes, and changing the other places that call PQconndefaults() to > > accept failures on either out of memory, or an invalid PGSERVICE > > > > Plan B: Create a new function PQconndefaults2(char * errorBuffer) or > > something similar that returned error information to the caller. > > > > Plan C: PQconndefaults() just ignores an invalid service but > > connection attempts fail because other callers of > > conninfo_add_defaults still pay attention to connection failures. > > This is the second patch I sent. > > > > Plan D: Service lookup failures are always ignored by > > conninfo_add_defaults. If you attempt to connect with a bad > > PGSERVICE set it will behave as if no PGSERVICE value was set. I > > don't think anyone explicitly proposed this yet. > > > > Plan 'D' is the only option that I'm opposed to, it will effect a > > lot more applications then ones that call PQconndefaults() and I > > feel it will confuse users. > > > > I'm not convinced plan B is worth the effort of having to maintain > > two versions of PQconndefaults() for a while to fix a corner case. > > OK, I am back to looking at this issue from March. I went with option > C, patch attached, which seems to be the most popular. It is similar to > Steve's patch, but structured differently and includes a doc patch. Patch applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +