From c2156c46ea0a35c74c54449f073b9b78adac0355 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 7 Apr 2015 15:07:45 +0000 Subject: [PATCH] Improve OOM detection of parseInput in libpq An OOM occurring at the beginning of parseInput() when called at the beginning of pqGetResult could result in libpq hanging indefinitely. The probability to face this case is rather low, but let's make the OOM error handling of parseInput more robust in this area. getCopyStart() has been made a bit smarter to make the difference between an OOM, meaning that input parsing should be immediately stopped, and cases where there is not enough data, meaning that parsing should continue. --- src/interfaces/libpq/fe-exec.c | 33 +++++++--- src/interfaces/libpq/fe-protocol2.c | 47 ++++++++------ src/interfaces/libpq/fe-protocol3.c | 123 ++++++++++++++++++++++++------------ src/interfaces/libpq/libpq-int.h | 4 +- 4 files changed, 134 insertions(+), 73 deletions(-) diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 828f18e..f979d4f 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -62,7 +62,7 @@ static int PQsendQueryGuts(PGconn *conn, const int *paramLengths, const int *paramFormats, int resultFormat); -static void parseInput(PGconn *conn); +static int parseInput(PGconn *conn); static PGresult *getCopyResult(PGconn *conn, ExecStatusType copytype); static bool PQexecStart(PGconn *conn); static PGresult *PQexecFinish(PGconn *conn); @@ -143,7 +143,11 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) result = (PGresult *) malloc(sizeof(PGresult)); if (!result) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("out of memory\n")); return NULL; + } result->ntups = 0; result->numAttributes = 0; @@ -194,6 +198,8 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) if (!result->events) { PQclear(result); + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("out of memory\n")); return NULL; } result->nEvents = conn->nEvents; @@ -1573,7 +1579,7 @@ pqHandleSendFailure(PGconn *conn) * Parse any available input messages. Since we are in PGASYNC_IDLE * state, only NOTICE and NOTIFY messages will be eaten. */ - parseInput(conn); + (void) parseInput(conn); } /* @@ -1641,14 +1647,16 @@ PQconsumeInput(PGconn *conn) * parseInput: if appropriate, parse input data from backend * until input is exhausted or a stopping state is reached. * Note that this function will NOT attempt to read more data from the backend. + * Exit: returns 0 if message is successfully parsed. + * returns EOF an error should occur. */ -static void +static int parseInput(PGconn *conn) { if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3) - pqParseInput3(conn); + return pqParseInput3(conn); else - pqParseInput2(conn); + return pqParseInput2(conn); } /* @@ -1663,7 +1671,8 @@ PQisBusy(PGconn *conn) return FALSE; /* Parse any available data, if our state permits. */ - parseInput(conn); + if (parseInput(conn)) + return FALSE; /* PQgetResult will return immediately in all states except BUSY. */ return conn->asyncStatus == PGASYNC_BUSY; @@ -1686,7 +1695,8 @@ PQgetResult(PGconn *conn) return NULL; /* Parse any available data, if our state permits. */ - parseInput(conn); + if (parseInput(conn)) + return NULL; /* If not ready to return something, block until we are. */ while (conn->asyncStatus == PGASYNC_BUSY) @@ -1721,7 +1731,8 @@ PQgetResult(PGconn *conn) } /* Parse it. */ - parseInput(conn); + if (parseInput(conn)) + return NULL; } /* Return the appropriate thing. */ @@ -2177,7 +2188,8 @@ PQnotifies(PGconn *conn) return NULL; /* Parse any available data to see if we can extract NOTIFY messages. */ - parseInput(conn); + if (parseInput(conn)) + return NULL; event = conn->notifyHead; if (event) @@ -2217,7 +2229,8 @@ PQputCopyData(PGconn *conn, const char *buffer, int nbytes) * input data into the input buffer happens down inside pqSendSome, but * it's not authorized to get rid of the data again.) */ - parseInput(conn); + if (parseInput(conn)) + return -1; if (nbytes > 0) { diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c index eeba7f3..aaa1905 100644 --- a/src/interfaces/libpq/fe-protocol2.c +++ b/src/interfaces/libpq/fe-protocol2.c @@ -406,8 +406,10 @@ error_return: * parseInput: if appropriate, parse input data from backend * until input is exhausted or a stopping state is reached. * Note that this function will NOT attempt to read more data from the backend. + * Exit: returns 0 if successfully parsed input. + * returns EOF an error should occur. */ -void +int pqParseInput2(PGconn *conn) { char id; @@ -424,14 +426,14 @@ pqParseInput2(PGconn *conn) * protocol and have identifying leading characters.) */ if (conn->asyncStatus == PGASYNC_COPY_OUT) - return; + return 0; /* * OK to try to read a message type code. */ conn->inCursor = conn->inStart; if (pqGetc(&id, conn)) - return; + return 0; /* * NOTIFY and NOTICE messages can happen in any state besides COPY @@ -447,18 +449,18 @@ pqParseInput2(PGconn *conn) if (id == 'A') { if (getNotify(conn)) - return; + return 0; } else if (id == 'N') { if (pqGetErrorNotice2(conn, false)) - return; + return 0; } else if (conn->asyncStatus != PGASYNC_BUSY) { /* If not IDLE state, just wait ... */ if (conn->asyncStatus != PGASYNC_IDLE) - return; + return 0; /* * Unexpected message in IDLE state; need to recover somehow. @@ -471,7 +473,7 @@ pqParseInput2(PGconn *conn) if (id == 'E') { if (pqGetErrorNotice2(conn, false /* treat as notice */ )) - return; + return 0; } else { @@ -492,13 +494,13 @@ pqParseInput2(PGconn *conn) { case 'C': /* command complete */ if (pqGets(&conn->workBuffer, conn)) - return; + return 0; if (conn->result == NULL) { conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + return EOF; } strlcpy(conn->result->cmdStatus, conn->workBuffer.data, CMDSTATUS_LEN); @@ -507,7 +509,7 @@ pqParseInput2(PGconn *conn) break; case 'E': /* error return */ if (pqGetErrorNotice2(conn, true)) - return; + return 0; conn->asyncStatus = PGASYNC_READY; break; case 'Z': /* backend is ready for new query */ @@ -516,7 +518,7 @@ pqParseInput2(PGconn *conn) case 'I': /* empty query */ /* read and throw away the closing '\0' */ if (pqGetc(&id, conn)) - return; + return 0; if (id != '\0') pqInternalNotice(&conn->noticeHooks, "unexpected character %c following empty query response (\"I\" message)", @@ -534,13 +536,13 @@ pqParseInput2(PGconn *conn) * Save the data and continue processing. */ if (pqGetInt(&(conn->be_pid), 4, conn)) - return; + return 0; if (pqGetInt(&(conn->be_key), 4, conn)) - return; + return 0; break; case 'P': /* synchronous (normal) portal */ if (pqGets(&conn->workBuffer, conn)) - return; + return 0; /* We pretty much ignore this message type... */ break; case 'T': /* row descriptions (start of query results) */ @@ -548,7 +550,7 @@ pqParseInput2(PGconn *conn) { /* First 'T' in a query sequence */ if (getRowDescriptions(conn)) - return; + return 0; /* getRowDescriptions() moves inStart itself */ continue; } @@ -562,7 +564,7 @@ pqParseInput2(PGconn *conn) * result. */ conn->asyncStatus = PGASYNC_READY; - return; + return 0; } break; case 'D': /* ASCII data tuple */ @@ -570,7 +572,7 @@ pqParseInput2(PGconn *conn) { /* Read another tuple of a normal query response */ if (getAnotherTuple(conn, FALSE)) - return; + return 0; /* getAnotherTuple() moves inStart itself */ continue; } @@ -580,7 +582,7 @@ pqParseInput2(PGconn *conn) "server sent data (\"D\" message) without prior row description (\"T\" message)"); /* Discard the unexpected message; good idea?? */ conn->inStart = conn->inEnd; - return; + return 0; } break; case 'B': /* Binary data tuple */ @@ -588,7 +590,7 @@ pqParseInput2(PGconn *conn) { /* Read another tuple of a normal query response */ if (getAnotherTuple(conn, TRUE)) - return; + return 0; /* getAnotherTuple() moves inStart itself */ continue; } @@ -598,7 +600,7 @@ pqParseInput2(PGconn *conn) "server sent binary data (\"B\" message) without prior row description (\"T\" message)"); /* Discard the unexpected message; good idea?? */ conn->inStart = conn->inEnd; - return; + return 0; } break; case 'G': /* Start Copy In */ @@ -622,12 +624,15 @@ pqParseInput2(PGconn *conn) /* Discard the unexpected message; good idea?? */ conn->inStart = conn->inEnd; conn->asyncStatus = PGASYNC_READY; - return; + return 0; } /* switch on protocol character */ } /* Successfully consumed this message */ conn->inStart = conn->inCursor; } + + /* we are done */ + return 0; } /* diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index a847f08..f27d193 100644 --- a/src/interfaces/libpq/fe-protocol3.c +++ b/src/interfaces/libpq/fe-protocol3.c @@ -61,8 +61,10 @@ static int build_startup_packet(const PGconn *conn, char *packet, * parseInput: if appropriate, parse input data from backend * until input is exhausted or a stopping state is reached. * Note that this function will NOT attempt to read more data from the backend. + * Exit: returns 0 if message is successfully parsed. + * returns EOF an error should occur. */ -void +int pqParseInput3(PGconn *conn) { char id; @@ -80,9 +82,9 @@ pqParseInput3(PGconn *conn) */ conn->inCursor = conn->inStart; if (pqGetc(&id, conn)) - return; + return 0; if (pqGetInt(&msgLength, 4, conn)) - return; + return 0; /* * Try to validate message type/length here. A length less than 4 is @@ -92,12 +94,12 @@ pqParseInput3(PGconn *conn) if (msgLength < 4) { handleSyncLoss(conn, id, msgLength); - return; + return 0; } if (msgLength > 30000 && !VALID_LONG_MESSAGE_TYPE(id)) { handleSyncLoss(conn, id, msgLength); - return; + return 0; } /* @@ -126,7 +128,7 @@ pqParseInput3(PGconn *conn) */ handleSyncLoss(conn, id, msgLength); } - return; + return 0; } /* @@ -148,18 +150,18 @@ pqParseInput3(PGconn *conn) if (id == 'A') { if (getNotify(conn)) - return; + return 0; } else if (id == 'N') { if (pqGetErrorNotice3(conn, false)) - return; + return 0; } else if (conn->asyncStatus != PGASYNC_BUSY) { /* If not IDLE state, just wait ... */ if (conn->asyncStatus != PGASYNC_IDLE) - return; + return 0; /* * Unexpected message in IDLE state; need to recover somehow. @@ -173,12 +175,12 @@ pqParseInput3(PGconn *conn) if (id == 'E') { if (pqGetErrorNotice3(conn, false /* treat as notice */ )) - return; + return 0; } else if (id == 'S') { if (getParameterStatus(conn)) - return; + return 0; } else { @@ -198,13 +200,13 @@ pqParseInput3(PGconn *conn) { case 'C': /* command complete */ if (pqGets(&conn->workBuffer, conn)) - return; + return 0; if (conn->result == NULL) { conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + return EOF; } strlcpy(conn->result->cmdStatus, conn->workBuffer.data, CMDSTATUS_LEN); @@ -212,12 +214,12 @@ pqParseInput3(PGconn *conn) break; case 'E': /* error return */ if (pqGetErrorNotice3(conn, true)) - return; + return 0; conn->asyncStatus = PGASYNC_READY; break; case 'Z': /* backend is ready for new query */ if (getReadyForQuery(conn)) - return; + return 0; conn->asyncStatus = PGASYNC_IDLE; break; case 'I': /* empty query */ @@ -226,7 +228,7 @@ pqParseInput3(PGconn *conn) conn->result = PQmakeEmptyPGresult(conn, PGRES_EMPTY_QUERY); if (!conn->result) - return; + return EOF; } conn->asyncStatus = PGASYNC_READY; break; @@ -239,7 +241,7 @@ pqParseInput3(PGconn *conn) conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + return EOF; } conn->asyncStatus = PGASYNC_READY; } @@ -250,7 +252,7 @@ pqParseInput3(PGconn *conn) break; case 'S': /* parameter status */ if (getParameterStatus(conn)) - return; + return 0; break; case 'K': /* secret key data from the backend */ @@ -260,9 +262,9 @@ pqParseInput3(PGconn *conn) * Save the data and continue processing. */ if (pqGetInt(&(conn->be_pid), 4, conn)) - return; + return 0; if (pqGetInt(&(conn->be_key), 4, conn)) - return; + return 0; break; case 'T': /* Row Description */ if (conn->result == NULL || @@ -270,7 +272,7 @@ pqParseInput3(PGconn *conn) { /* First 'T' in a query sequence */ if (getRowDescriptions(conn, msgLength)) - return; + return EOF; /* getRowDescriptions() moves inStart itself */ continue; } @@ -284,7 +286,7 @@ pqParseInput3(PGconn *conn) * result. */ conn->asyncStatus = PGASYNC_READY; - return; + return 0; } break; case 'n': /* No Data */ @@ -306,14 +308,14 @@ pqParseInput3(PGconn *conn) conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + return EOF; } conn->asyncStatus = PGASYNC_READY; } break; case 't': /* Parameter Description */ if (getParamDescriptions(conn)) - return; + return 0; break; case 'D': /* Data Row */ if (conn->result != NULL && @@ -321,7 +323,7 @@ pqParseInput3(PGconn *conn) { /* Read another tuple of a normal query response */ if (getAnotherTuple(conn, msgLength)) - return; + return EOF; /* getAnotherTuple() moves inStart itself */ continue; } @@ -345,21 +347,36 @@ pqParseInput3(PGconn *conn) } break; case 'G': /* Start Copy In */ - if (getCopyStart(conn, PGRES_COPY_IN)) - return; - conn->asyncStatus = PGASYNC_COPY_IN; + { + int res = getCopyStart(conn, PGRES_COPY_IN); + if (res == -1) + return 0; + else if (res == -2) + return EOF; + conn->asyncStatus = PGASYNC_COPY_IN; + } break; case 'H': /* Start Copy Out */ - if (getCopyStart(conn, PGRES_COPY_OUT)) - return; - conn->asyncStatus = PGASYNC_COPY_OUT; - conn->copy_already_done = 0; + { + int res = getCopyStart(conn, PGRES_COPY_OUT); + if (res == -1) + return 0; + else if (res == -2) + return EOF; + conn->asyncStatus = PGASYNC_COPY_OUT; + conn->copy_already_done = 0; + } break; case 'W': /* Start Copy Both */ - if (getCopyStart(conn, PGRES_COPY_BOTH)) - return; - conn->asyncStatus = PGASYNC_COPY_BOTH; - conn->copy_already_done = 0; + { + int res = getCopyStart(conn, PGRES_COPY_BOTH); + if (res == -1) + return 0; + else if (res == -2) + return EOF; + conn->asyncStatus = PGASYNC_COPY_BOTH; + conn->copy_already_done = 0; + } break; case 'd': /* Copy Data */ @@ -412,6 +429,9 @@ pqParseInput3(PGconn *conn) conn->inStart += 5 + msgLength; } } + + /* we are done */ + return 0; } /* @@ -466,7 +486,8 @@ getRowDescriptions(PGconn *conn, int msgLength) result = PQmakeEmptyPGresult(conn, PGRES_TUPLES_OK); if (!result) { - errmsg = NULL; /* means "out of memory", see below */ + /* Error is already saved by PQmakeEmptyPGresult() */ + errmsg = conn->errorMessage.data; goto advance_and_error; } @@ -1309,6 +1330,11 @@ getNotify(PGconn *conn) * CopyBothResponse message * * parseInput already read the message type and length. + * Returns: 0 in case of success. + * -1 if there is not enough data. In this case parsing should + * continue later on. + * -2 in case of an OOM error. In this case parsing should be + * suspended immediately. */ static int getCopyStart(PGconn *conn, ExecStatusType copytype) @@ -1316,17 +1342,27 @@ getCopyStart(PGconn *conn, ExecStatusType copytype) PGresult *result; int nfields; int i; + int res = 0; result = PQmakeEmptyPGresult(conn, copytype); if (!result) + { + res = -2; goto failure; + } if (pqGetc(&conn->copy_is_binary, conn)) + { + res = -1; goto failure; + } result->binary = conn->copy_is_binary; /* the next two bytes are the number of fields */ if (pqGetInt(&(result->numAttributes), 2, conn)) + { + res = -1; goto failure; + } nfields = result->numAttributes; /* allocate space for the attribute descriptors */ @@ -1335,7 +1371,10 @@ getCopyStart(PGconn *conn, ExecStatusType copytype) result->attDescs = (PGresAttDesc *) pqResultAlloc(result, nfields * sizeof(PGresAttDesc), TRUE); if (!result->attDescs) + { + res = -2; goto failure; + } MemSet(result->attDescs, 0, nfields * sizeof(PGresAttDesc)); } @@ -1344,7 +1383,10 @@ getCopyStart(PGconn *conn, ExecStatusType copytype) int format; if (pqGetInt(&format, 2, conn)) + { + res = -1; goto failure; + } /* * Since pqGetInt treats 2-byte integers as unsigned, we need to @@ -1356,11 +1398,12 @@ getCopyStart(PGconn *conn, ExecStatusType copytype) /* Success! */ conn->result = result; - return 0; + return res; failure: - PQclear(result); - return EOF; + if (result) + PQclear(result); + return res; } /* diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 2175957..235af89 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -559,7 +559,7 @@ extern PostgresPollingStatusType pqSetenvPoll(PGconn *conn); extern char *pqBuildStartupPacket2(PGconn *conn, int *packetlen, const PQEnvironmentOption *options); -extern void pqParseInput2(PGconn *conn); +extern int pqParseInput2(PGconn *conn); extern int pqGetCopyData2(PGconn *conn, char **buffer, int async); extern int pqGetline2(PGconn *conn, char *s, int maxlen); extern int pqGetlineAsync2(PGconn *conn, char *buffer, int bufsize); @@ -573,7 +573,7 @@ extern PGresult *pqFunctionCall2(PGconn *conn, Oid fnid, extern char *pqBuildStartupPacket3(PGconn *conn, int *packetlen, const PQEnvironmentOption *options); -extern void pqParseInput3(PGconn *conn); +extern int pqParseInput3(PGconn *conn); extern int pqGetErrorNotice3(PGconn *conn, bool isError); extern int pqGetCopyData3(PGconn *conn, char **buffer, int async); extern int pqGetline3(PGconn *conn, char *s, int maxlen); -- 2.3.5