Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index b75f553..7d2aa35
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** check_pghost_envvar(void)
*** 325,330
--- 325,333
start = PQconndefaults();
+ if (!start)
+ pg_fatal(out of memory\n);
+
for (option = start; option-keyword != NULL; option++)
{
if (option-envvar (strcmp(option-envvar, PGHOST) == 0 ||
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 955f248..503a63a
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** typedef struct
*** 483,489
with an entry having a null structfieldkeyword/ pointer. The
null pointer is returned if memory could not be allocated. Note that
the current default values (structfieldval/structfield fields)
!will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
/para
--- 483,490
with an entry having a null structfieldkeyword/ pointer. The
null pointer is returned if memory could not be allocated. Note that
the current default values (structfieldval/structfield fields)
!will depend on environment variables and other context. A
!missing or invalid service file will be silently ignored. Callers
must treat the connection options data as read-only.
/para
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 975f795..9797140
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*** pg_fe_sendauth(AuthRequest areq, PGconn
*** 982,988
* if there is an error, return NULL with an error message in errorMessage
*/
char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
{
const char *name = NULL;
char *authn;
--- 982,988
* if there is an error, return NULL with an error message in errorMessage
*/
char *
! pg_fe_getauthname(void)
{
const char *name = NULL;
char *authn;
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
new file mode 100644
index bfddc68..25440b0
*** 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(PQExpBuffer errorMessage);
#endif /* FE_AUTH_H */
--- 19,24
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 8dd1a59..7ab4a9a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** PQconndefaults(void)
*** 875,881
connOptions = conninfo_init(errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, errorBuf))
{
PQconninfoFree(connOptions);
connOptions = NULL;
--- 875,882
connOptions = conninfo_init(errorBuf);
if (connOptions != NULL)
{
! /* pass NULL errorBuf to ignore errors */
! if (!conninfo_add_defaults(connOptions, NULL))
{
PQconninfoFree(connOptions);
connOptions = NULL;
***
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
On 13-03-26 12:40 AM, Tom Lane wrote: Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
On 13-03-20 05:54 PM, Tom Lane wrote:
Steve Singer ssin...@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
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index ed67759..c1d8d69 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** check_pghost_envvar(void)
*** 300,306
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option-keyword != NULL; option++)
{
if (option-envvar (strcmp(option-envvar, PGHOST) == 0 ||
--- 300,309
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (start == NULL)
! {
! pg_log(PG_FATAL,can not get default connection options out of memory\n);
! }
for (option = start; option-keyword != NULL; option++)
{
if (option-envvar (strcmp(option-envvar, PGHOST) == 0 ||
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index eea9c6b..0a9a29e 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** static PQconninfoOption *conninfo_array_
*** 347,353
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
static bool conninfo_add_defaults(PQconninfoOption *options,
! PQExpBuffer errorMessage);
static PQconninfoOption *conninfo_uri_parse(const char *uri,
PQExpBuffer errorMessage, bool use_defaults);
static bool conninfo_uri_parse_options(PQconninfoOption *options,
--- 347,354
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
static bool conninfo_add_defaults(PQconninfoOption *options,
! PQExpBuffer errorMessage,
! bool ignore_missing_service);
static PQconninfoOption *conninfo_uri_parse(const char *uri,
PQExpBuffer errorMessage, bool use_defaults);
static bool conninfo_uri_parse_options(PQconninfoOption *options,
*** PQconndefaults(void)
*** 875,881
connOptions = conninfo_init(errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, errorBuf))
{
PQconninfoFree(connOptions);
connOptions = NULL;
--- 876,882
connOptions = conninfo_init(errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, errorBuf,true))
{
PQconninfoFree(connOptions);
connOptions = NULL;
*** conninfo_parse(const char *conninfo, PQE
*** 4243,4249
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
--- 4244,4250
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage,false))
{
PQconninfoFree(options);
return NULL;
*** conninfo_array_parse(const char *const *
*** 4395,4401
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
--- 4396,4402
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage,false))
{
PQconninfoFree(options);
return NULL;
*** conninfo_array_parse(const char *const *
*** 4416,4422
* error condition here --- we just leave the option's value as NULL.
*/
static bool
! conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
{
PQconninfoOption *option;
char *tmp;
--- 4417,4424
* error condition here --- we just leave the option's value as NULL.
*/
static bool
! conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage,
! bool ignore_missing_service)
{
PQconninfoOption *option;
char *tmp;
*** conninfo_add_defaults(PQconninfoOption *
*** 4425,4431
* If there's a service spec, use it to obtain any not-explicitly-given
* parameters.
*/
! if (parseServiceInfo(options, errorMessage) != 0)
return false;
/*
---
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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 ssin...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
new file mode 100644
index bba0d2a..eca061f
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*** dblink_fdw_validator(PG_FUNCTION_ARGS)
*** 1947,1953
if (!options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg(out of memory),
errdetail(could not get libpq's default connection options)));
}
--- 1947,1953
if (!options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg(out of memory or service name cannot be found),
errdetail(could not get libpq's default connection options)));
}
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index ed67759..cd246ce
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** check_pghost_envvar(void)
*** 300,306
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option-keyword != NULL; option++)
{
if (option-envvar (strcmp(option-envvar, PGHOST) == 0 ||
--- 300,310
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (!start)
! pg_log(PG_FATAL,
! out of memory or service name cannot be found\n
! could not get libpq's default connection options\n);
!
for (option = start; option-keyword != NULL; option++)
{
if (option-envvar (strcmp(option-envvar, PGHOST) == 0 ||
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
new file mode 100644
index 123cb4f..d4890be
*** a/contrib/postgres_fdw/option.c
--- b/contrib/postgres_fdw/option.c
*** InitPgFdwOptions(void)
*** 169,175
if (!libpq_options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg(out of memory),
errdetail(could not get libpq's default connection options)));
/* Count how many libpq options are available. */
--- 169,175
if (!libpq_options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg(out of memory or service name cannot be found),
errdetail(could not get libpq's default connection options)));
/* Count how many libpq options are available. */
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 775d250..ceb873e
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** typedef struct
*** 481,487
current default values. The return value points to an array of
structnamePQconninfoOption/structname structures, which ends
with an entry having a null structfieldkeyword/ pointer. The
!null pointer is returned if memory could not be allocated. Note that
the current default values (structfieldval/structfield fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
--- 481,488
current default values. The return value points to an array of
structnamePQconninfoOption/structname structures, which ends
with an entry having a null structfieldkeyword/ pointer. The
!null pointer is returned if memory could not be allocated or
!the specified connection service name cannot be found. Note that
the current default values (structfieldval/structfield fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
new file mode 100644
index 4ba257d..f02f4f9
*** a/src/bin/scripts/pg_isready.c
--- b/src/bin/scripts/pg_isready.c
*** main(int argc, char **argv)
***
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
On Wed, Mar 20, 2013 at 01:30:20PM -0400, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
Steve Singer ssin...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
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. 0x0040bdd1 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 functionPQconnectdb/function options and their current default values. The return value points to an array of structnamePQconninfoOption/structname structures, which ends with an entry having a null structfieldkeyword/ pointer. The --null pointer is returned if memory could not be allocated. Note that the current default values (structfieldval/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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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. 0x0040bdd1 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 functionPQconnectdb/function options and their current default values. The return value points to an array of structnamePQconninfoOption/structname structures, which ends with an entry having a null structfieldkeyword/ pointer. The --null pointer is returned if memory could not be allocated. Note that the current default values (structfieldval/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? * 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 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. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
