Fix handling of invalid sockets returned by PQsocket()
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?
The patch attached addresses those issues.
This has been raised in this message, but beginning a new thread makes
more sense:
/messages/by-id/CAB7nPqTTZoiuVYGNonLVnZysStUSOfhKeO9FTrQbKWJ36UCdOA@mail.gmail.com
Regards,
--
Michael
Attachments:
pqsocket-error-fix.patchtext/x-patch; charset=US-ASCII; name=pqsocket-error-fix.patchDownload
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 832f9f9..b4325b0 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -360,6 +360,14 @@ StreamLogicalLog(void)
struct timeval timeout;
struct timeval *timeoutptr = NULL;
+ if (PQsocket(conn) < 0)
+ {
+ fprintf(stderr,
+ _("%s: bad socket: \"%s\"\n"),
+ progname, strerror(errno));
+ goto error;
+ }
+
FD_ZERO(&input_mask);
FD_SET(PQsocket(conn), &input_mask);
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c6afcd5..e81123f 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -70,7 +70,7 @@ static void DisconnectDatabase(ParallelSlot *slot);
static int select_loop(int maxFd, fd_set *workerset, bool *aborting);
-static void init_slot(ParallelSlot *slot, PGconn *conn);
+static void init_slot(ParallelSlot *slot, PGconn *conn, const char *progname);
static void help(const char *progname);
@@ -421,14 +421,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
* array contains the connection.
*/
slots = (ParallelSlot *) pg_malloc(sizeof(ParallelSlot) * concurrentCons);
- init_slot(slots, conn);
+ init_slot(slots, conn, progname);
if (parallel)
{
for (i = 1; i < concurrentCons; i++)
{
conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false, true);
- init_slot(slots + i, conn);
+ init_slot(slots + i, conn, progname);
}
}
@@ -917,11 +917,18 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
}
static void
-init_slot(ParallelSlot *slot, PGconn *conn)
+init_slot(ParallelSlot *slot, PGconn *conn, const char *progname)
{
slot->connection = conn;
slot->isFree = true;
slot->sock = PQsocket(conn);
+
+ if (slot->sock < 0)
+ {
+ fprintf(stderr, _("%s: bad socket: %s\n"), progname,
+ strerror(errno));
+ exit(1);
+ }
}
static void
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 0a9d25c..3e13a39 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -720,6 +720,12 @@ try_complete_step(Step *step, int flags)
PGresult *res;
bool canceled = false;
+ if (sock < 0)
+ {
+ fprintf(stderr, "bad socket: %s\n", strerror(errno));
+ exit_nicely();
+ }
+
gettimeofday(&start_time, NULL);
FD_ZERO(&read_set);
Michael Paquier wrote:
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?
I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Michael Paquier wrote:
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.
OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.
--
Michael
Attachments:
pqsocket-error-fix-v2.patchbinary/octet-stream; name=pqsocket-error-fix-v2.patchDownload
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 832f9f9..456cb4f 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -360,6 +360,14 @@ StreamLogicalLog(void)
struct timeval timeout;
struct timeval *timeoutptr = NULL;
+ if (PQsocket(conn) < 0)
+ {
+ fprintf(stderr,
+ _("%s: bad socket: \"%s\"\n"),
+ progname, PQerrorMessage(conn));
+ goto error;
+ }
+
FD_ZERO(&input_mask);
FD_SET(PQsocket(conn), &input_mask);
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 6d7e635..2e721e8 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -956,7 +956,8 @@ CopyStreamPoll(PGconn *conn, long timeout_ms)
if (PQsocket(conn) < 0)
{
- fprintf(stderr, _("%s: socket not open"), progname);
+ fprintf(stderr, _("%s: socket not open: %s\n"), progname,
+ PQerrorMessage(conn));
return -1;
}
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c6afcd5..08a7468 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -70,7 +70,7 @@ static void DisconnectDatabase(ParallelSlot *slot);
static int select_loop(int maxFd, fd_set *workerset, bool *aborting);
-static void init_slot(ParallelSlot *slot, PGconn *conn);
+static void init_slot(ParallelSlot *slot, PGconn *conn, const char *progname);
static void help(const char *progname);
@@ -421,14 +421,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
* array contains the connection.
*/
slots = (ParallelSlot *) pg_malloc(sizeof(ParallelSlot) * concurrentCons);
- init_slot(slots, conn);
+ init_slot(slots, conn, progname);
if (parallel)
{
for (i = 1; i < concurrentCons; i++)
{
conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false, true);
- init_slot(slots + i, conn);
+ init_slot(slots + i, conn, progname);
}
}
@@ -917,11 +917,18 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
}
static void
-init_slot(ParallelSlot *slot, PGconn *conn)
+init_slot(ParallelSlot *slot, PGconn *conn, const char *progname)
{
slot->connection = conn;
slot->isFree = true;
slot->sock = PQsocket(conn);
+
+ if (slot->sock < 0)
+ {
+ fprintf(stderr, _("%s: bad socket: %s\n"), progname,
+ PQerrorMessage(conn));
+ exit(1);
+ }
}
static void
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 0a9d25c..a955911 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -720,6 +720,12 @@ try_complete_step(Step *step, int flags)
PGresult *res;
bool canceled = false;
+ if (sock < 0)
+ {
+ fprintf(stderr, "bad socket: %s\n", PQerrorMessage(conn));
+ exit_nicely();
+ }
+
gettimeofday(&start_time, NULL);
FD_ZERO(&read_set);
On 2/17/16 10:52 PM, Michael Paquier wrote:
On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Michael Paquier wrote:
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.
Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut wrote:
On 2/17/16 10:52 PM, Michael Paquier wrote:
On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Michael Paquier wrote:
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.
You're completely right. Let's patch pgbench that way too.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Mar 6, 2016 at 12:52 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Peter Eisentraut wrote:
On 2/17/16 10:52 PM, Michael Paquier wrote:
On Thu, Feb 18, 2016 at 1:58 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Michael Paquier wrote:
Hi all,
After looking at Alvaro's message mentioning the handling of
PQsocket() for invalid sockets, I just had a look by curiosity at
other calls of this routine, and found a couple of issues:
1) In vacuumdb.c, init_slot() does not check for the return value of PQsocket():
slot->sock = PQsocket(conn);
2) In isolationtester.c, try_complete_step() should do the same.
3) In pg_recvlogical.c for StreamLogicalLog() I am spotting the same problem.
I guess those ones should be fixed as well, no?I patched pgbench to use PQerrorMessage rather than strerror(errno). I
think your patch should do the same.OK, this looks like a good idea. I would suggest doing the same in
receivelog.c then.Let's make the error messages consistent as "invalid socket". "bad
socket" isn't really our style, and pg_basebackup saying "socket not
open" is just plain incorrect.You're completely right. Let's patch pgbench that way too.
Here is v3 then, switching to "invalid socket" for those error
messages. There are two extra messages in fe-misc.c and
libpqwalreceiver.c that need a rewording that I have detected as well.
--
Michael
Attachments:
pqsocket-error-fix-v3.patchapplication/x-patch; name=pqsocket-error-fix-v3.patchDownload
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index f670957..4ee4d71 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -331,7 +331,7 @@ libpq_select(int timeout_ms)
if (PQsocket(streamConn) < 0)
ereport(ERROR,
(errcode_for_socket_access(),
- errmsg("socket not open")));
+ errmsg("invalid socket: %s", PQerrorMessage(streamConn))));
/* We use poll(2) if available, otherwise select(2) */
{
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 832f9f9..6d12705 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -360,6 +360,14 @@ StreamLogicalLog(void)
struct timeval timeout;
struct timeval *timeoutptr = NULL;
+ if (PQsocket(conn) < 0)
+ {
+ fprintf(stderr,
+ _("%s: invalid socket: %s"),
+ progname, PQerrorMessage(conn));
+ goto error;
+ }
+
FD_ZERO(&input_mask);
FD_SET(PQsocket(conn), &input_mask);
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 6d7e635..01c42fc 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -956,7 +956,8 @@ CopyStreamPoll(PGconn *conn, long timeout_ms)
if (PQsocket(conn) < 0)
{
- fprintf(stderr, _("%s: socket not open"), progname);
+ fprintf(stderr, _("%s: invalid socket: %s"), progname,
+ PQerrorMessage(conn));
return -1;
}
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 8b0b17a..92df750 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3797,7 +3797,7 @@ threadRun(void *arg)
sock = PQsocket(st->con);
if (sock < 0)
{
- fprintf(stderr, "bad socket: %s", PQerrorMessage(st->con));
+ fprintf(stderr, "invalid socket: %s", PQerrorMessage(st->con));
goto done;
}
@@ -3867,7 +3867,8 @@ threadRun(void *arg)
if (sock < 0)
{
- fprintf(stderr, "bad socket: %s", PQerrorMessage(st->con));
+ fprintf(stderr, "invalid socket: %s",
+ PQerrorMessage(st->con));
goto done;
}
if (FD_ISSET(sock, &input_mask) ||
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c6afcd5..b673be8 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -70,7 +70,7 @@ static void DisconnectDatabase(ParallelSlot *slot);
static int select_loop(int maxFd, fd_set *workerset, bool *aborting);
-static void init_slot(ParallelSlot *slot, PGconn *conn);
+static void init_slot(ParallelSlot *slot, PGconn *conn, const char *progname);
static void help(const char *progname);
@@ -421,14 +421,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
* array contains the connection.
*/
slots = (ParallelSlot *) pg_malloc(sizeof(ParallelSlot) * concurrentCons);
- init_slot(slots, conn);
+ init_slot(slots, conn, progname);
if (parallel)
{
for (i = 1; i < concurrentCons; i++)
{
conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, false, true);
- init_slot(slots + i, conn);
+ init_slot(slots + i, conn, progname);
}
}
@@ -917,11 +917,18 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
}
static void
-init_slot(ParallelSlot *slot, PGconn *conn)
+init_slot(ParallelSlot *slot, PGconn *conn, const char *progname)
{
slot->connection = conn;
slot->isFree = true;
slot->sock = PQsocket(conn);
+
+ if (slot->sock < 0)
+ {
+ fprintf(stderr, _("%s: invalid socket: %s"), progname,
+ PQerrorMessage(conn));
+ exit(1);
+ }
}
static void
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 30cee7f..32da8ca 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -1058,7 +1058,7 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)
if (conn->sock == PGINVALID_SOCKET)
{
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("socket not open\n"));
+ libpq_gettext("invalid socket\n"));
return -1;
}
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 6461ae8..2969ce9 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -705,6 +705,12 @@ try_complete_step(Step *step, int flags)
PGresult *res;
bool canceled = false;
+ if (sock < 0)
+ {
+ fprintf(stderr, "invalid socket: %s", PQerrorMessage(conn));
+ exit_nicely();
+ }
+
gettimeofday(&start_time, NULL);
FD_ZERO(&read_set);
Michael Paquier wrote:
Here is v3 then, switching to "invalid socket" for those error
messages. There are two extra messages in fe-misc.c and
libpqwalreceiver.c that need a rewording that I have detected as well.
Peter Eisentraut pushed this as a40814d7a.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers