innocuous: pgbench does FD_ISSET on invalid socket

Started by Alvaro Herreraalmost 10 years ago6 messages

alvherre@alvh.no-ip.org

almost 10 years ago

I noticed that pgbench calls FD_ISSET on a socket returned by
PQsocket() without first checking that it's not invalid. I don't think
there's a real problem here because the socket was already checked a few
lines above, but I think applying the same coding pattern to both places
is cleaner.

Any objections to changing it like this? I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
***************
*** 3770,3780 **** threadRun(void *arg)
  			Command   **commands = sql_script[st->use_file].commands;
  			int			prev_ecnt = st->ecnt;

! if (st->con && (FD_ISSET(PQsocket(st->con), &input_mask)
! || commands[st->state]->type == META_COMMAND))
{
! if (!doCustom(thread, st, &aggs))
! remains--; /* I've aborted */
}

  			if (st->ecnt > prev_ecnt && commands[st->state]->type == META_COMMAND)
--- 3770,3790 ----
  			Command   **commands = sql_script[st->use_file].commands;
  			int			prev_ecnt = st->ecnt;

! if (st->con)
{
! int sock = PQsocket(st->con);
!
! if (sock < 0)
! {
! fprintf(stderr, "bad socket: %s\n", strerror(errno));
! goto done;
! }
! if (FD_ISSET(sock, &input_mask) ||
! commands[st->state]->type == META_COMMAND)
! {
! if (!doCustom(thread, st, &aggs))
! remains--; /* I've aborted */
! }
}

if (st->ecnt > prev_ecnt && commands[st->state]->type == META_COMMAND)

--
ï¿½lvaro Herrera http://www.linkedin.com/in/alvherre

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

almost 10 years ago

In reply to: Alvaro Herrera (#1)

Re: innocuous: pgbench does FD_ISSET on invalid socket

On Sat, Feb 13, 2016 at 6:25 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I noticed that pgbench calls FD_ISSET on a socket returned by
PQsocket() without first checking that it's not invalid. I don't think
there's a real problem here because the socket was already checked a few
lines above, but I think applying the same coding pattern to both places
is cleaner.

Any objections to changing it like this? I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

Not really, +1 for consistency here, and this makes the code clearer.

Different issues in the same area:
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?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

almost 10 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: innocuous: pgbench does FD_ISSET on invalid socket

On Mon, Feb 15, 2016 at 3:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Feb 13, 2016 at 6:25 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I noticed that pgbench calls FD_ISSET on a socket returned by
PQsocket() without first checking that it's not invalid. I don't think
there's a real problem here because the socket was already checked a few
lines above, but I think applying the same coding pattern to both places
is cleaner.

Any objections to changing it like this? I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

Not really, +1 for consistency here, and this makes the code clearer.

Different issues in the same area:
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?

With a patch, that's even better.
--
Michael

Attachments:

pqsocket-error-fix.patchtext/x-diff; 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);

Fabien COELHO

coelho@cri.ensmp.fr

almost 10 years ago

In reply to: Alvaro Herrera (#1)

Re: innocuous: pgbench does FD_ISSET on invalid socket

Hello Alvaro,

Any objections to changing it like this? I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

My 0.02ï¿œ: if the pattern is repeated, maybe a function which incorporates
the check would save lines and improve overall readability?

... = safePQsocket(...);

--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera

alvherre@2ndquadrant.com

almost 10 years ago

In reply to: Fabien COELHO (#4)

Re: innocuous: pgbench does FD_ISSET on invalid socket

I noticed that strerror(errno) wasn't the most helpful error context
ever, so I changed it to PQerrorMessage(). There may be room for
additional improvement there.

I also noticed that if one connection dies, we terminate the whole
thread, and if the thread is serving multiple connections, the other
ones are not PQfinished. It may or may not be worthwhile improving
this; if pgbench is used to test the server when connections are
randomly dropped that would be helpful, otherwise not so much.

I ended up backpatching all the way back.

Fabien COELHO wrote:

Hello Alvaro,

Any objections to changing it like this? I'd probably backpatch to 9.5,
but no further (even though this pattern actually appears all the way
back.)

My 0.02€: if the pattern is repeated, maybe a function which incorporates
the check would save lines and improve overall readability?

... = safePQsocket(...);

Hmm, yeah, but that doesn't work too well because we're not invoking
exit() in case of an error (which is what the safe pg_malloc etc do), so
we'd have to check for what to do after safePQsocket returns -- no
improvement in code clarity IMHO. Thanks for the suggestion.

Michael Paquier wrote:

Different issues in the same area:
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?

Hmm, yeah, perhaps it's worth closing these 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

Import Notes

Reply to msg id not found: alpine.DEB.2.10.1602151152450.18755@stoCAB7nPqTTZoiuVYGNonLVnZysStUSOfhKeO9FTrQbKWJ36UCdOA@mail.gmail.com | Resolved by subject fallback

Michael Paquier

michael.paquier@gmail.com

almost 10 years ago

In reply to: Alvaro Herrera (#5)

Re: innocuous: pgbench does FD_ISSET on invalid socket

On Tue, Feb 16, 2016 at 8:47 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Different issues in the same area:
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?

Hmm, yeah, perhaps it's worth closing these too.

Do you think that it would be better starting a new thread? The only
difficulty of this patch is to be sure that each error handling is
done correctly for each one of those frontend modules.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers