From 1e8e2d2691662a1238632b1732c0f0c0f1d07853 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 8 Apr 2015 13:16:20 +0900 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() and getParamDescriptions() are 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 later on. --- src/interfaces/libpq/fe-exec.c | 27 +++--- src/interfaces/libpq/fe-protocol2.c | 54 +++++++----- src/interfaces/libpq/fe-protocol3.c | 159 ++++++++++++++++++++++++++---------- src/interfaces/libpq/libpq-int.h | 4 +- 4 files changed, 166 insertions(+), 78 deletions(-) diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 828f18e..754d7f3 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); @@ -1573,7 +1573,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 +1641,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 if an error occurs. */ -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 +1665,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 +1689,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 +1725,8 @@ PQgetResult(PGconn *conn) } /* Parse it. */ - parseInput(conn); + if (parseInput(conn)) + return NULL; } /* Return the appropriate thing. */ @@ -2177,7 +2182,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 +2223,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..66d631e 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 if an error occurs. */ -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; + goto failure; } 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)", @@ -524,6 +526,8 @@ pqParseInput2(PGconn *conn) if (conn->result == NULL) conn->result = PQmakeEmptyPGresult(conn, PGRES_EMPTY_QUERY); + if (conn->result == NULL) + goto failure; conn->asyncStatus = PGASYNC_READY; break; case 'K': /* secret key data from the backend */ @@ -534,13 +538,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 +552,7 @@ pqParseInput2(PGconn *conn) { /* First 'T' in a query sequence */ if (getRowDescriptions(conn)) - return; + return 0; /* getRowDescriptions() moves inStart itself */ continue; } @@ -562,7 +566,7 @@ pqParseInput2(PGconn *conn) * result. */ conn->asyncStatus = PGASYNC_READY; - return; + return 0; } break; case 'D': /* ASCII data tuple */ @@ -570,7 +574,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 +584,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 +592,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 +602,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 +626,20 @@ 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; + +failure: + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("out of memory\n")); + return EOF; } /* diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index a847f08..17580e6 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 if an error occurs. */ -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; + goto failure; } 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; + goto failure; } conn->asyncStatus = PGASYNC_READY; break; @@ -239,7 +241,7 @@ pqParseInput3(PGconn *conn) conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + goto failure; } 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,19 @@ pqParseInput3(PGconn *conn) conn->result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!conn->result) - return; + goto failure; } conn->asyncStatus = PGASYNC_READY; } break; case 't': /* Parameter Description */ - if (getParamDescriptions(conn)) - return; + { + int res = getParamDescriptions(conn); + if (res == -1) + return 0; + else if (res == -2) + goto failure; + } break; case 'D': /* Data Row */ if (conn->result != NULL && @@ -321,7 +328,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 +352,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) + goto failure; + 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) + goto failure; + 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) + goto failure; + conn->asyncStatus = PGASYNC_COPY_BOTH; + conn->copy_already_done = 0; + } break; case 'd': /* Copy Data */ @@ -412,6 +434,14 @@ pqParseInput3(PGconn *conn) conn->inStart += 5 + msgLength; } } + + /* we are done */ + return 0; + +failure: + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("out of memory\n")); + return EOF; } /* @@ -466,7 +496,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; } @@ -613,7 +644,9 @@ advance_and_error: /* * parseInput subroutine to read a 't' (ParameterDescription) message. * We'll build a new PGresult structure containing the parameter data. - * Returns: 0 if completed message, EOF if not enough data yet. + * Returns: 0 if completed message + * -1 if not enough data yet. + * -2 in case of an OOM error. * * Note that if we run out of data, we have to release the partially * constructed PGresult, and rebuild it again next time. Fortunately, @@ -625,15 +658,22 @@ getParamDescriptions(PGconn *conn) PGresult *result; int nparams; int i; + int res = 0; result = PQmakeEmptyPGresult(conn, PGRES_COMMAND_OK); if (!result) + { + res = -2; goto failure; + } /* parseInput already read the 't' label and message length. */ /* the next two bytes are the number of parameters */ if (pqGetInt(&(result->numParameters), 2, conn)) + { + res = -1; goto failure; + } nparams = result->numParameters; /* allocate space for the parameter descriptors */ @@ -642,7 +682,10 @@ getParamDescriptions(PGconn *conn) result->paramDescs = (PGresParamDesc *) pqResultAlloc(result, nparams * sizeof(PGresParamDesc), TRUE); if (!result->paramDescs) + { + res = -2; goto failure; + } MemSet(result->paramDescs, 0, nparams * sizeof(PGresParamDesc)); } @@ -652,17 +695,21 @@ getParamDescriptions(PGconn *conn) int typid; if (pqGetInt(&typid, 4, conn)) + { + res = -1; goto failure; + } result->paramDescs[i].typid = typid; } /* Success! */ conn->result = result; - return 0; + return res; failure: - PQclear(result); - return EOF; + if (result) + PQclear(result); + return res; } /* @@ -1309,6 +1356,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 +1368,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 +1397,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 +1409,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 +1424,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.4