diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c index 62de5c19250..7e02fcc2452 100644 --- a/src/interfaces/libpq/fe-misc.c +++ b/src/interfaces/libpq/fe-misc.c @@ -56,6 +56,8 @@ static int pqSendSome(PGconn *conn, int len); static int pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time); static int pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time); +static int pqReadData_internal(PGconn *conn); +static int pqDrainPending(PGconn *conn); /* * PQlibVersion: return the libpq version number @@ -579,6 +581,13 @@ pqPutMsgEnd(PGconn *conn) /* ---------- * pqReadData: read more data, if any is available + * + * Upon a successful return, callers may assume that either 1) all available + * bytes have been consumed from the socket, or 2) the socket is still marked + * readable by the OS. (In other words: after a successful pqReadData, it's + * safe to tell a client to poll for readable bytes on the socket without any + * further draining of the SSL/GSS transport buffers.) + * * Possible return values: * 1: successfully loaded at least one more byte * 0: no data is presently available, but no error detected @@ -591,8 +600,7 @@ pqPutMsgEnd(PGconn *conn) int pqReadData(PGconn *conn) { - int someread = 0; - int nread; + int available; if (conn->sock == PGINVALID_SOCKET) { @@ -600,6 +608,40 @@ pqReadData(PGconn *conn) return -1; } + available = pqReadData_internal(conn); + if (available < 0) + return -1; + else if (available > 0) + { + /* + * Make sure there are no bytes stuck in layers between conn->inBuffer + * and the socket, to make it safe for clients to poll on PQsocket(). + */ + if (pqDrainPending(conn)) + return -1; + } + else + { + /* + * If we're not returning any bytes from the underlying transport, + * that must imply there aren't any in the transport buffer... + */ + Assert(pqsecure_bytes_pending(conn) == 0); + } + + return available; +} + +/* + * Workhorse for pqReadData(). It's kept separate from the pqDrainPending() + * logic to avoid adding to this function's goto complexity. + */ +static int +pqReadData_internal(PGconn *conn) +{ + int someread = 0; + int nread; + /* Left-justify any data in the buffer to make room */ if (conn->inStart < conn->inEnd) { @@ -786,6 +828,105 @@ definitelyFailed: return -1; } +/*--- + * Drain any transport data that is already buffered in userspace and add it + * to conn->inBuffer, enlarging inBuffer if necessary. The drain fails if + * inBuffer cannot be made to hold all available transport data. + * + * We assume that the underlying secure transport implementation does not + * attempt to read any more data from the socket while draining the transport + * buffer. After a successful return, pqsecure_bytes_pending() must be zero. + * + * This operation is necessary to prevent deadlock, due to a layering + * violation designed into our asynchronous client API: pqReadData() and all + * the parsing routines above it receive data from the SSL/GSS transport + * buffer, but clients poll on the raw PQsocket() handle. So data can be + * "lost" in the intermediate layer if we don't take it out here. + * + * To illustrate what we're trying to prevent, say that the server is sending + * two messages at once in response to a query (Aaaa and Bb), the libpq buffer + * is five characters in size, and TLS records max out at three-character + * payloads. Here's what would happen if pqReadData() didn't call + * pqDrainPending(): + * + * Client libpq SSL Socket + * | | | | + * | [ ] [ ] [ ] [1] Buffers are empty, client is + * x --------------------------> | polling on socket + * | | | | + * | [ ] [ ] [xxx] [2] First record is received; poll + * | <-------------------------- | signals read-ready + * | | | | + * x ---> [ ] [ ] [xxx] [3] Client calls PQconsumeInput() + * | | | | + * | [ ] -> [ ] [xxx] [4] libpq calls pqReadData() to fill + * | | | | the receive buffer + * | [ ] [Aaa] <-- [ ] [5] SSL pulls payload off the wire + * | | | | and decrypts it + * | [Aaa ] <- [ ] [ ] [6] pqsecure_read() takes all data + * | | | | + * | <--- [Aaa ] [ ] [ ] [7] PQconsumeInput() returns with a + * x --------------------------> | partial message, PQisBusy() is + * | | | | still true, client polls again + * | [Aaa ] [ ] [xxx] [8] Second record is received; poll + * | <-------------------------- | signals read-ready + * | | | | + * x ---> [Aaa ] [ ] [xxx] [9] Client calls PQconsumeInput() + * | | | | + * | [Aaa ] -> [ ] [xxx] [10] libpq calls pqReadData() to fill + * | | | | the receive buffer + * | [Aaa ] [aBb] <-- [ ] [11] SSL decrypts + * | | | | + * | [AaaaB] <- [b ] [ ] [12] pqsecure_read() fills its + * | | | | buffer, taking only two bytes + * | <--- [AaaaB] [b ] [ ] [13] PQconsumeInput() returns with a + * | | | | complete message buffered; + * | | | | PQisBusy() is false + * x ---> [AaaaB] [b ] [ ] [14] Client calls PQgetResult() + * | | | | + * | <--- [B ] [b ] [ ] [15] Aaaa is returned; PQisBusy() is + * x --------------------------> | true and client polls again + * . | | . + * . [B ] [b ] . [16] No packets, and client hangs. + * . | | . + * + * The pqDrainPending() call fixes the above scenario at step [13]. Before + * returning to the Client, it first expands the libpq buffer and moves the + * remaining data from the SSL buffer to the libpq buffer. + * + * The function returns 0 on success and -1 on error. Success means that + * there was no data pending or it was successfully drained to conn->inBuffer. + * On error, conn->errorMessage is set. + */ +static int +pqDrainPending(PGconn *conn) +{ + ssize_t bytes_pending; + ssize_t nread; + + bytes_pending = pqsecure_bytes_pending(conn); + if (bytes_pending <= 0) + return bytes_pending; + + /* Expand the input buffer if necessary. */ + if (pqCheckInBufferSpace(conn->inEnd + (size_t) bytes_pending, conn)) + return -1; /* errorMessage already set */ + + nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd, + bytes_pending); + conn->inEnd += nread; + + /* When there are bytes pending, the read function is not supposed to fail */ + if (nread != bytes_pending) + { + libpq_append_conn_error(conn, + "drained only %zu of %zd pending bytes in transport buffer", + nread, bytes_pending); + return -1; + } + return 0; +} + /* * pqSendSome: send data waiting in the output buffer. * diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index b400a28965e..dd6c4ca56ed 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -265,7 +265,9 @@ pgtls_bytes_pending(PGconn *conn) int pending; /* - * OpenSSL readahead is documented to break SSL_pending(). + * OpenSSL readahead is documented to break SSL_pending(). Plus, we can't + * afford to have OpenSSL take bytes off the socket without processing + * them; that breaks the postconditions for pqsecure_drain_pending(). */ Assert(!SSL_get_read_ahead(conn->ssl)); diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 3c54bf6a76f..42e8dc0d331 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -284,7 +284,8 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len) * Return the number of bytes available in the transport buffer. * * If pqsecure_read() is called for this number of bytes, it's guaranteed to - * return successfully without reading from the underlying socket. + * return successfully without reading from the underlying socket. See + * pqDrainPending() for a more complete discussion of the concepts involved. */ ssize_t pqsecure_bytes_pending(PGconn *conn)