Re: [BUGS] [PATCH] Prevent hanging on unreachable hosts on startup
On Mon, Aug 27, 2012 at 09:34:48PM -0400, Tom Lane wrote: > Bruce Momjian writes: > > Tom, can you comment on this patch because you commented on the previous > > version? Thanks. > > Doesn't that provoke a pile of compiler warnings about local variables > potentially being altered during longjmp? It sure looks pretty unsafe > from here. It also fails to print any error message if a connection > attempt is canceled. > > A bigger-picture question is just how safe it is to longjmp out of libpq > partway through a connection attempt. At the very least, that's likely > to leak an open socket and some memory. Maybe that doesn't matter too > much for psql, but I'm not convinced. It seems fairly likely that > internal state for libc (malloc), openssl, etc, could be left in a > corrupt state if the interrupt happens at just the wrong time. (This > didn't matter so much for the original proposal, where we'd just have > been exiting psql anyway on failure. But if it's in \c then we want > psql to still be operational after a failure.) > > We could perhaps dodge the longjmp safety question by using a > PQconnectStartParams/PQconnectPoll loop. I'm not sure that provides > 100% the same functionality --- in particular, I think it wouldn't allow > interrupting out of a DNS name resolution delay. But it'd be less > likely to result in strange crashes. > > regards, tom lane The latest on this patch can be found here: http://archives.postgresql.org/message-id/500151c4.5010...@enterprisedb.com -Ryan Kelly -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] [PATCH] Prevent hanging on unreachable hosts on startup
Bruce Momjian writes: > Tom, can you comment on this patch because you commented on the previous > version? Thanks. Doesn't that provoke a pile of compiler warnings about local variables potentially being altered during longjmp? It sure looks pretty unsafe from here. It also fails to print any error message if a connection attempt is canceled. A bigger-picture question is just how safe it is to longjmp out of libpq partway through a connection attempt. At the very least, that's likely to leak an open socket and some memory. Maybe that doesn't matter too much for psql, but I'm not convinced. It seems fairly likely that internal state for libc (malloc), openssl, etc, could be left in a corrupt state if the interrupt happens at just the wrong time. (This didn't matter so much for the original proposal, where we'd just have been exiting psql anyway on failure. But if it's in \c then we want psql to still be operational after a failure.) We could perhaps dodge the longjmp safety question by using a PQconnectStartParams/PQconnectPoll loop. I'm not sure that provides 100% the same functionality --- in particular, I think it wouldn't allow interrupting out of a DNS name resolution delay. But it'd be less likely to result in strange crashes. regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] [PATCH] Prevent hanging on unreachable hosts on startup
On Thu, Jan 5, 2012 at 09:23:55AM -0500, Ryan Kelly wrote:
> On Wed, Jan 04, 2012 at 09:36:57PM -0500, Tom Lane wrote:
> > "Ryan P. Kelly" writes:
> > > The signal handler installed by setup_cancel_handler() will ignore
> > > attempts to exit psql should a host be unreachable.
> >
> > Hm. That may be worth doing something about, but the proposed patch
> > seems more like a quick-and-dirty hack than a solution. The main
> > thing I don't like about it is that if we care about this scenario
> > during initial startup, we should also care about it during a \c
> > command, but just delaying the installation of the signal handler
> > won't fix that case.
> Sure, but do you still think moving the signal handler down is correct?
> I've attached another patch that handles interrupting \c. I also noticed
> there appears to be what looks like duplication of code in startup.c and
> command.c in main() and do_connect() respectively. I'm wondering if I
> they should be made to share the same code which (with my patch) would
> then mean the signal handler could be left where it was.
>
> > More generally, what if the server becomes unreachable mid-session?
> > I'm not sure how much there is to be done about that case, since
> > there's probably no good way to distinguish it from a query that
> > takes a really long time. But if we're going to think about this,
> > we might as well think about all the cases.
> Well in this case we can still interrupt the query, no? This will drop
> us back into the prompt, where we can safely \q.
Tom, can you comment on this patch because you commented on the previous
version? Thanks.
---
> >From 8f9d0b5088021d944aceac65a96d7bd0c24aa0c6 Mon Sep 17 00:00:00 2001
> From: "Ryan P. Kelly"
> Date: Thu, 5 Jan 2012 09:13:38 -0500
> Subject: [PATCH] Allow interrupting hanging \c connection attempts
>
> ---
> src/bin/psql/command.c | 17 +
> 1 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
> index 69fac83..845705d 100644
> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -1516,7 +1516,7 @@ static bool
> do_connect(char *dbname, char *user, char *host, char *port)
> {
> PGconn *o_conn = pset.db,
> -*n_conn;
> +*n_conn = NULL;
> char *password = NULL;
>
> if (!dbname)
> @@ -1570,7 +1570,13 @@ do_connect(char *dbname, char *user, char *host, char
> *port)
> keywords[7] = NULL;
> values[7] = NULL;
>
> - n_conn = PQconnectdbParams(keywords, values, true);
> + if (sigsetjmp(sigint_interrupt_jmp, 1) != 0) {
> + /* got here with longjmp */
> + } else {
> + sigint_interrupt_enabled = true;
> + n_conn = PQconnectdbParams(keywords, values, true);
> + sigint_interrupt_enabled = false;
> + }
>
> free(keywords);
> free(values);
> @@ -1600,7 +1606,8 @@ do_connect(char *dbname, char *user, char *host, char
> *port)
>*/
> if (pset.cur_cmd_interactive)
> {
> - psql_error("%s", PQerrorMessage(n_conn));
> + if (n_conn)
> + psql_error("%s", PQerrorMessage(n_conn));
>
> /* pset.db is left unmodified */
> if (o_conn)
> @@ -1608,7 +1615,9 @@ do_connect(char *dbname, char *user, char *host, char
> *port)
> }
> else
> {
> - psql_error("\\connect: %s", PQerrorMessage(n_conn));
> + if (n_conn)
> + psql_error("\\connect: %s",
> PQerrorMessage(n_conn));
> +
> if (o_conn)
> {
> PQfinish(o_conn);
> --
> 1.7.7.4
>
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] [PATCH] Prevent hanging on unreachable hosts on startup
On Wed, Jan 04, 2012 at 09:36:57PM -0500, Tom Lane wrote:
> "Ryan P. Kelly" writes:
> > The signal handler installed by setup_cancel_handler() will ignore
> > attempts to exit psql should a host be unreachable.
>
> Hm. That may be worth doing something about, but the proposed patch
> seems more like a quick-and-dirty hack than a solution. The main
> thing I don't like about it is that if we care about this scenario
> during initial startup, we should also care about it during a \c
> command, but just delaying the installation of the signal handler
> won't fix that case.
Sure, but do you still think moving the signal handler down is correct?
I've attached another patch that handles interrupting \c. I also noticed
there appears to be what looks like duplication of code in startup.c and
command.c in main() and do_connect() respectively. I'm wondering if I
they should be made to share the same code which (with my patch) would
then mean the signal handler could be left where it was.
> More generally, what if the server becomes unreachable mid-session?
> I'm not sure how much there is to be done about that case, since
> there's probably no good way to distinguish it from a query that
> takes a really long time. But if we're going to think about this,
> we might as well think about all the cases.
Well in this case we can still interrupt the query, no? This will drop
us back into the prompt, where we can safely \q.
>
> regards, tom lane
-Ryan Kelly
>From 8f9d0b5088021d944aceac65a96d7bd0c24aa0c6 Mon Sep 17 00:00:00 2001
From: "Ryan P. Kelly"
Date: Thu, 5 Jan 2012 09:13:38 -0500
Subject: [PATCH] Allow interrupting hanging \c connection attempts
---
src/bin/psql/command.c | 17 +
1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 69fac83..845705d 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1516,7 +1516,7 @@ static bool
do_connect(char *dbname, char *user, char *host, char *port)
{
PGconn *o_conn = pset.db,
- *n_conn;
+ *n_conn = NULL;
char *password = NULL;
if (!dbname)
@@ -1570,7 +1570,13 @@ do_connect(char *dbname, char *user, char *host, char *port)
keywords[7] = NULL;
values[7] = NULL;
- n_conn = PQconnectdbParams(keywords, values, true);
+ if (sigsetjmp(sigint_interrupt_jmp, 1) != 0) {
+ /* got here with longjmp */
+ } else {
+ sigint_interrupt_enabled = true;
+ n_conn = PQconnectdbParams(keywords, values, true);
+ sigint_interrupt_enabled = false;
+ }
free(keywords);
free(values);
@@ -1600,7 +1606,8 @@ do_connect(char *dbname, char *user, char *host, char *port)
*/
if (pset.cur_cmd_interactive)
{
- psql_error("%s", PQerrorMessage(n_conn));
+ if (n_conn)
+psql_error("%s", PQerrorMessage(n_conn));
/* pset.db is left unmodified */
if (o_conn)
@@ -1608,7 +1615,9 @@ do_connect(char *dbname, char *user, char *host, char *port)
}
else
{
- psql_error("\\connect: %s", PQerrorMessage(n_conn));
+ if (n_conn)
+psql_error("\\connect: %s", PQerrorMessage(n_conn));
+
if (o_conn)
{
PQfinish(o_conn);
--
1.7.7.4
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Re: [BUGS] [PATCH] Prevent hanging on unreachable hosts on startup
"Ryan P. Kelly" writes: > The signal handler installed by setup_cancel_handler() will ignore > attempts to exit psql should a host be unreachable. Hm. That may be worth doing something about, but the proposed patch seems more like a quick-and-dirty hack than a solution. The main thing I don't like about it is that if we care about this scenario during initial startup, we should also care about it during a \c command, but just delaying the installation of the signal handler won't fix that case. More generally, what if the server becomes unreachable mid-session? I'm not sure how much there is to be done about that case, since there's probably no good way to distinguish it from a query that takes a really long time. But if we're going to think about this, we might as well think about all the cases. regards, tom lane -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
