Bug #889: PQconnectdb SEGV

Chris Brown (cmb-postgres(at)chibi(dot)ca) reports a bug with a severity of 2
The lower the number the more severe it is.

Short Description
PQconnectdb SEGV

Long Description
In the libpq C interface, PQconnectdb results in a SEGV when the conn->sock number is greater than __FD_SETSIZE (1024 on linux). The crash is caused by stack corruption when attempting to FD_SET.

Solution: switch to poll() whenever that is available. Example code is a unified diff. It needs some cleaning, and autoconf work, but the bulk of the code is written.

Also, this patch is still being tested, but after 24hrs has held up without crashing. If possible, I would like to know which version of PG this is likely to go in to (in whatever form it takes).

Side Note: pqWriteReady and pqReadReady could simply be calls to pqWait. I didn't code it that way to minimize impact. That change would reduce the number of code paths to test.

Sample Code
diff -ru postgresql-7.3.orig/src/interfaces/libpq/fe-misc.c postgresql-7.3.poll/src/interfaces/libpq/fe-misc.c
--- postgresql-7.3.orig/src/interfaces/libpq/fe-misc.c Thu Oct 24 16:35:55 2002
+++ postgresql-7.3.poll/src/interfaces/libpq/fe-misc.c Wed Jan 29 12:11:47 2003
@@ -43,9 +43,13 @@
#include <sys/time.h>
#endif

+#if USE_SELECT_INSTEAD_OF_POLL // {
#ifdef HAVE_SYS_SELECT_H
#include <sys/select.h>
#endif
+#else // }{ if USE_SELECT_INSTEAD_OF_POLL
+#include <sys/poll.h>
+#endif // } if USE_SELECT_INSTEAD_OF_POLL else

#include "libpq-fe.h"
#include "libpq-int.h"
@@ -350,6 +354,8 @@
return 0;
}

+
+#if USE_SELECT_INSTEAD_OF_POLL // {
/*
* pqReadReady: is select() saying the file is ready to read?
* JAB: -or- if SSL is enabled and used, is it buffering bytes?
@@ -426,6 +432,107 @@
}
return FD_ISSET(conn->sock, &input_mask) ? 1 : 0;
}
+#else // }{ if USE_SELECT_INSTEAD_OF_POLL
+/*
+ * pqReadReady: is poll() saying the file is ready to read?
+ * JAB: -or- if SSL is enabled and used, is it buffering bytes?
+ * Returns -1 on failure, 0 if not ready, 1 if ready.
+ */
+int
+pqReadReady(PGconn *conn)
+{
+ struct pollfd input_fd;
+
+ if (!conn || conn->sock < 0)
+ return -1;
+
+/* JAB: Check for SSL library buffering read bytes */
+#ifdef USE_SSL
+ if (conn->ssl && SSL_pending(conn->ssl) > 0)
+ {
+ /* short-circuit the select */
+ return 1;
+ }
+#endif
+
+ input_fd.fd = conn->sock;
+ input_fd.events = POLLIN;
+ input_fd.revents = 0;
+
+ while ( poll( &input_fd, 1, -1 ) < 0 )
+ {
+ if ( SOCK_ERRNO == EINTR )
+ {
+ /* Interrupted system call - we'll just try again */
+ continue;
+ }
+
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("poll() failed: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO));
+ return -1;
+ }
+
+ if ( input_fd.revents | POLLIN )
+ {
+ return 1;
+ }
+ else
+ {
+ return 0;
+ }
+}
+
+/*
+ * pqWriteReady: is poll() saying the file is ready to write?
+ * Returns -1 on failure, 0 if not ready, 1 if ready.
+ */
+int
+pqWriteReady(PGconn *conn)
+{
+ struct pollfd input_fd;
+
+ if (!conn || conn->sock < 0)
+ return -1;
+
+/* JAB: Check for SSL library buffering read bytes */
+#ifdef USE_SSL
+ if (conn->ssl && SSL_pending(conn->ssl) > 0)
+ {
+ /* short-circuit the select */
+ return 1;
+ }
+#endif
+
+ input_fd.fd = conn->sock;
+ input_fd.events = POLLOUT;
+ input_fd.revents = 0;
+
+ while ( poll( &input_fd, 1, -1 ) < 0 )
+ {
+ if ( SOCK_ERRNO == EINTR )
+ {
+ /* Interrupted system call - we'll just try again */
+ continue;
+ }
+
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("poll() failed: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO));
+ return -1;
+ }
+
+ if ( input_fd.revents | POLLOUT )
+ {
+ return 1;
+ }
+ else
+ {
+ return 0;
+ }
+}
+#endif // } if USE_SELECT_INSTEAD_OF_POLL else
+

/* ----------
* pqReadData: read more data, if any is available
@@ -782,6 +889,8 @@
return pqWaitTimed(forRead, forWrite, conn, (time_t) -1);
}

+
+#if USE_SELECT_INSTEAD_OF_POLL // {
/*
* pqWaitTimed: wait, but not past finish_time.
*
@@ -867,7 +976,100 @@

return 0;
}
+#else // }{ if USE_SELECT_INSTEAD_OF_POLL
+/*
+ * pqWaitTimed: wait, but not past finish_time.
+ *
+ * If finish_time is exceeded then we return failure (EOF). This is different
+ * from the response for a kernel exception (return 0) because we don't want
+ * the caller to try to read/write in that case.
+ *
+ * finish_time = ((time_t) -1) disables the wait limit.
+ */
+int
+pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
+{
+ struct pollfd input_fd;
+ int timeout_ms;
+ int poll_result;
+
+ if (conn->sock < 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("connection not open\n"));
+ return EOF;
+ }
+
+/* JAB: Check for SSL library buffering read bytes */
+#ifdef USE_SSL
+ if (forRead && conn->ssl && SSL_pending(conn->ssl) > 0)
+ {
+ /* short-circuit the select */
+ return 0;
+ }
+#endif
+
+ if ( !forRead && !forWrite )
+ {
+ return 0;
+ }
+
+ do
+ {
+ input_fd.fd = conn->sock;
+ input_fd.events = 0;
+ input_fd.revents = 0;
+ if (forRead)
+ {
+ input_fd.events |= POLLIN;
+ }
+ if (forWrite)
+ {
+ input_fd.events |= POLLOUT;
+ }
+ timeout_ms = -1;
+
+ if (finish_time != (time_t)-1 )
+ {
+ /*
+ * Set up delay. Assume caller incremented finish_time
+ * so that we can error out as soon as time() passes it.
+ * Note we will recalculate delay each time through the loop.
+ */
+ time_t now = time(NULL);
+
+ if (finish_time > now)
+ {
+ timeout_ms = ( finish_time - now ) * 1000;
+ }
+ else
+ {
+ timeout_ms = 0;
+ }
+ }
+
+ poll_result = poll( &input_fd, 1, timeout_ms );
+ }
+ while ( poll_result < 0 && SOCK_ERRNO == EINTR );

+ if ( poll_result < 0 )
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("select() failed: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO));
+ return EOF;
+ }
+
+ if ( poll_result == 0 )
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("timeout expired\n"));
+ return EOF;
+ }
+
+ return 0;
+}
+#endif // } if USE_SELECT_INSTEAD_OF_POLL else


/*
Binary files postgresql-7.3.orig/src/interfaces/libpq/fe-misc.o and postgresql-7.3.poll/src/interfaces/libpq/fe-misc.o differ
Binary files postgresql-7.3.orig/src/interfaces/libpq/libpq.a and postgresql-7.3.poll/src/interfaces/libpq/libpq.a differ
Binary files postgresql-7.3.orig/src/interfaces/libpq/libpq.so and postgresql-7.3.poll/src/interfaces/libpq/libpq.so differ
Binary files postgresql-7.3.orig/src/interfaces/libpq/libpq.so.2 and postgresql-7.3.poll/src/interfaces/libpq/libpq.so.2 differ
Binary files postgresql-7.3.orig/src/interfaces/libpq/libpq.so.2.2 and postgresql-7.3.poll/src/interfaces/libpq/libpq.so.2.2 differ

No file was uploaded with this report