Fix missed cases in libpq's error handling.

tglsfdc · tglsfdc · commit 914611ea738a · 2022-04-21T17:12:49.000-04:00
Commit 618c167 invented an "error_result" flag in PGconn, which intends to represent the state that we have an error condition and need to build a PGRES_FATAL_ERROR PGresult from the message text in conn->errorMessage, but have not yet done so. (Postponing construction of the error object simplifies dealing with out-of-memory conditions and with concatenation of messages for multiple errors.) For nearly all purposes, this "virtual" PGresult object should act the same as if it were already materialized. But a couple of places in fe-protocol3.c didn't get that memo, and were only testing conn->result as they used to, without also checking conn->error_result. In hopes of reducing the probability of similar mistakes in future, I invented a pgHavePendingResult() macro that includes both tests. Per report from Peter Eisentraut. Discussion: https://postgr.es/m/b52277b9-fa66-b027-4a37-fb8989c73ff8@enterprisedb.com
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
@@ -1196,6 +1196,7 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value)
  *	  Returns 1 if OK, 0 if error occurred.
  *
  * On error, *errmsgp can be set to an error string to be returned.
+ * (Such a string should already be translated via libpq_gettext().)
  * If it is left NULL, the error is presumed to be "out of memory".
  *
  * In single-row mode, we create a new result holding just the current row,
@@ -1986,7 +1987,7 @@ PQsetSingleRowMode(PGconn *conn)
 		(conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE &&
 		 conn->cmd_queue_head->queryclass != PGQUERY_EXTENDED))
 		return 0;
-	if (conn->result || conn->error_result)
+	if (pgHavePendingResult(conn))
 		return 0;
 
 	/* OK, set flag */
@@ -2941,7 +2942,7 @@ PQfn(PGconn *conn,
 	}
 
 	if (conn->sock == PGINVALID_SOCKET || conn->asyncStatus != PGASYNC_IDLE ||
-		conn->result || conn->error_result)
+		pgHavePendingResult(conn))
 	{
 		appendPQExpBufferStr(&conn->errorMessage,
 							 libpq_gettext("connection in wrong state\n"));
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
@@ -209,7 +209,7 @@ pqParseInput3(PGconn *conn)
 				case 'C':		/* command complete */
 					if (pqGets(&conn->workBuffer, conn))
 						return;
-					if (conn->result == NULL)
+					if (!pgHavePendingResult(conn))
 					{
 						conn->result = PQmakeEmptyPGresult(conn,
 														   PGRES_COMMAND_OK);
@@ -263,7 +263,7 @@ pqParseInput3(PGconn *conn)
 					}
 					break;
 				case 'I':		/* empty query */
-					if (conn->result == NULL)
+					if (!pgHavePendingResult(conn))
 					{
 						conn->result = PQmakeEmptyPGresult(conn,
 														   PGRES_EMPTY_QUERY);
@@ -281,7 +281,7 @@ pqParseInput3(PGconn *conn)
 					if (conn->cmd_queue_head &&
 						conn->cmd_queue_head->queryclass == PGQUERY_PREPARE)
 					{
-						if (conn->result == NULL)
+						if (!pgHavePendingResult(conn))
 						{
 							conn->result = PQmakeEmptyPGresult(conn,
 															   PGRES_COMMAND_OK);
@@ -362,7 +362,7 @@ pqParseInput3(PGconn *conn)
 					if (conn->cmd_queue_head &&
 						conn->cmd_queue_head->queryclass == PGQUERY_DESCRIBE)
 					{
-						if (conn->result == NULL)
+						if (!pgHavePendingResult(conn))
 						{
 							conn->result = PQmakeEmptyPGresult(conn,
 															   PGRES_COMMAND_OK);
@@ -2133,7 +2133,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
 				 * report COMMAND_OK.  Otherwise, the backend violated the
 				 * protocol, so complain.
 				 */
-				if (!(conn->result || conn->error_result))
+				if (!pgHavePendingResult(conn))
 				{
 					if (status == PGRES_COMMAND_OK)
 					{
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
@@ -852,6 +852,14 @@ extern void pqTraceOutputNoTypeByteMessage(PGconn *conn, const char *message);
 	(resetPQExpBuffer(&(conn)->errorMessage), \
 	 (conn)->errorReported = 0)
 
+/*
+ * Check whether we have a PGresult pending to be returned --- either a
+ * constructed one in conn->result, or a "virtual" error result that we
+ * don't intend to materialize until the end of the query cycle.
+ */
+#define pgHavePendingResult(conn) \
+	((conn)->result != NULL || (conn)->error_result)
+
 /*
  * this is so that we can check if a connection is non-blocking internally
  * without the overhead of a function call

Original file line number	Diff line number	Diff line change
`@@ -1196,6 +1196,7 @@ pqSaveParameterStatus(PGconn conn, const char name, const char *value)`
`1196`	`1196`	`* Returns 1 if OK, 0 if error occurred.`
`1197`	`1197`	`*`
`1198`	`1198`	`* On error, *errmsgp can be set to an error string to be returned.`
	`1199`	`+ * (Such a string should already be translated via libpq_gettext().)`
`1199`	`1200`	`* If it is left NULL, the error is presumed to be "out of memory".`
`1200`	`1201`	`*`
`1201`	`1202`	`* In single-row mode, we create a new result holding just the current row,`
`@@ -1986,7 +1987,7 @@ PQsetSingleRowMode(PGconn *conn)`
`1986`	`1987`	`(conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE &&`
`1987`	`1988`	`conn->cmd_queue_head->queryclass != PGQUERY_EXTENDED))`
`1988`	`1989`	`return 0;`
`1989`		`- if (conn->result \|\| conn->error_result)`
	`1990`	`+ if (pgHavePendingResult(conn))`
`1990`	`1991`	`return 0;`
`1991`	`1992`
`1992`	`1993`	`/* OK, set flag */`
`@@ -2941,7 +2942,7 @@ PQfn(PGconn *conn,`
`2941`	`2942`	`}`
`2942`	`2943`
`2943`	`2944`	`if (conn->sock == PGINVALID_SOCKET \|\| conn->asyncStatus != PGASYNC_IDLE \|\|`
`2944`		`- conn->result \|\| conn->error_result)`
	`2945`	`+ pgHavePendingResult(conn))`
`2945`	`2946`	`{`
`2946`	`2947`	`appendPQExpBufferStr(&conn->errorMessage,`
`2947`	`2948`	`libpq_gettext("connection in wrong state\n"));`
Original file line number	Diff line number	Diff line change
`@@ -209,7 +209,7 @@ pqParseInput3(PGconn *conn)`
`209`	`209`	`case 'C': /* command complete */`
`210`	`210`	`if (pqGets(&conn->workBuffer, conn))`
`211`	`211`	`return;`
`212`		`- if (conn->result == NULL)`
	`212`	`+ if (!pgHavePendingResult(conn))`
`213`	`213`	`{`
`214`	`214`	`conn->result = PQmakeEmptyPGresult(conn,`
`215`	`215`	`PGRES_COMMAND_OK);`
`@@ -263,7 +263,7 @@ pqParseInput3(PGconn *conn)`
`263`	`263`	`}`
`264`	`264`	`break;`
`265`	`265`	`case 'I': /* empty query */`
`266`		`- if (conn->result == NULL)`
	`266`	`+ if (!pgHavePendingResult(conn))`
`267`	`267`	`{`
`268`	`268`	`conn->result = PQmakeEmptyPGresult(conn,`
`269`	`269`	`PGRES_EMPTY_QUERY);`
`@@ -281,7 +281,7 @@ pqParseInput3(PGconn *conn)`
`281`	`281`	`if (conn->cmd_queue_head &&`
`282`	`282`	`conn->cmd_queue_head->queryclass == PGQUERY_PREPARE)`
`283`	`283`	`{`
`284`		`- if (conn->result == NULL)`
	`284`	`+ if (!pgHavePendingResult(conn))`
`285`	`285`	`{`
`286`	`286`	`conn->result = PQmakeEmptyPGresult(conn,`
`287`	`287`	`PGRES_COMMAND_OK);`
`@@ -362,7 +362,7 @@ pqParseInput3(PGconn *conn)`
`362`	`362`	`if (conn->cmd_queue_head &&`
`363`	`363`	`conn->cmd_queue_head->queryclass == PGQUERY_DESCRIBE)`
`364`	`364`	`{`
`365`		`- if (conn->result == NULL)`
	`365`	`+ if (!pgHavePendingResult(conn))`
`366`	`366`	`{`
`367`	`367`	`conn->result = PQmakeEmptyPGresult(conn,`
`368`	`368`	`PGRES_COMMAND_OK);`
`@@ -2133,7 +2133,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,`
`2133`	`2133`	`* report COMMAND_OK. Otherwise, the backend violated the`
`2134`	`2134`	`* protocol, so complain.`
`2135`	`2135`	`*/`
`2136`		`- if (!(conn->result \|\| conn->error_result))`
	`2136`	`+ if (!pgHavePendingResult(conn))`
`2137`	`2137`	`{`
`2138`	`2138`	`if (status == PGRES_COMMAND_OK)`
`2139`	`2139`	`{`