In libpq, always append new error messages to conn->errorMessage.

Previously, we had an undisciplined mish-mash of printfPQExpBuffer and
appendPQExpBuffer calls to report errors within libpq.  This commit
establishes a uniform rule that appendPQExpBuffer[Str] should be used.
conn->errorMessage is reset only at the start of an application request,
and then accumulates messages till we're done.  We can remove no less
than three different ad-hoc mechanisms that were used to get the effect
of concatenation of error messages within a sequence of operations.

Although this makes things quite a bit cleaner conceptually, the main
reason to do it is to make the world safer for the multiple-target-host
feature that was added awhile back.  Previously, there were many cases
in which an error occurring during an individual host connection attempt
would wipe out the record of what had happened during previous attempts.
(The reporting is still inadequate, in that it can be hard to tell which
host got the failure, but that seems like a matter for a separate commit.)

Currently, lo_import and lo_export contain exceptions to the "never
use printfPQExpBuffer" rule.  If we changed them, we'd risk reporting
an incidental lo_close failure before the actual read or write
failure, which would be confusing, not least because lo_close happened
after the main failure.  We could improve this by inventing an
internal version of lo_close that doesn't reset the errorMessage; but
we'd also need a version of PQfn() that does that, and it didn't quite
seem worth the trouble for now.

Discussion: https://postgr.es/m/BN6PR05MB3492948E4FD76C156E747E8BC9160@BN6PR05MB3492.namprd05.prod.outlook.com

1 files changed, 89 insertions, 80 deletions

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index a25fe4dd17a..168b3df52bf 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -72,7 +72,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
 		ginbuf.value = malloc(payloadlen);
 		if (!ginbuf.value)
 		{
-			printfPQExpBuffer(&conn->errorMessage,
+			appendPQExpBuffer(&conn->errorMessage,
 							  libpq_gettext("out of memory allocating GSSAPI buffer (%d)\n"),
 							  payloadlen);
 			return STATUS_ERROR;
@@ -154,15 +154,15 @@ pg_GSS_startup(PGconn *conn, int payloadlen)
 
 	if (!(host && host[0] != '\0'))
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("host name must be specified\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("host name must be specified\n"));
 		return STATUS_ERROR;
 	}
 
 	if (conn->gctx)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("duplicate GSS authentication request\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("duplicate GSS authentication request\n"));
 		return STATUS_ERROR;
 	}
 
@@ -195,10 +195,10 @@ pg_SSPI_error(PGconn *conn, const char *mprefix, SECURITY_STATUS r)
 					  FORMAT_MESSAGE_FROM_SYSTEM,
 					  NULL, r, 0,
 					  sysmsg, sizeof(sysmsg), NULL) == 0)
-		printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
+		appendPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
 						  mprefix, (unsigned int) r);
 	else
-		printfPQExpBuffer(&conn->errorMessage, "%s: %s (%x)\n",
+		appendPQExpBuffer(&conn->errorMessage, "%s: %s (%x)\n",
 						  mprefix, sysmsg, (unsigned int) r);
 }
 
@@ -226,7 +226,7 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
 		inputbuf = malloc(payloadlen);
 		if (!inputbuf)
 		{
-			printfPQExpBuffer(&conn->errorMessage,
+			appendPQExpBuffer(&conn->errorMessage,
 							  libpq_gettext("out of memory allocating SSPI buffer (%d)\n"),
 							  payloadlen);
 			return STATUS_ERROR;
@@ -286,7 +286,8 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
 		conn->sspictx = malloc(sizeof(CtxtHandle));
 		if (conn->sspictx == NULL)
 		{
-			printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("out of memory\n"));
 			return STATUS_ERROR;
 		}
 		memcpy(conn->sspictx, &newContext, sizeof(CtxtHandle));
@@ -305,7 +306,8 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
 			 * authentication. Keep check in case it shows up with other
 			 * authentication methods later.
 			 */
-			printfPQExpBuffer(&conn->errorMessage, "SSPI returned invalid number of output buffers\n");
+			appendPQExpBufferStr(&conn->errorMessage,
+								 "SSPI returned invalid number of output buffers\n");
 			return STATUS_ERROR;
 		}
 
@@ -345,8 +347,8 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
 
 	if (conn->sspictx)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("duplicate SSPI authentication request\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("duplicate SSPI authentication request\n"));
 		return STATUS_ERROR;
 	}
 
@@ -356,7 +358,8 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
 	conn->sspicred = malloc(sizeof(CredHandle));
 	if (conn->sspicred == NULL)
 	{
-		printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("out of memory\n"));
 		return STATUS_ERROR;
 	}
 
@@ -384,14 +387,15 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
 	 */
 	if (!(host && host[0] != '\0'))
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("host name must be specified\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("host name must be specified\n"));
 		return STATUS_ERROR;
 	}
 	conn->sspitarget = malloc(strlen(conn->krbsrvname) + strlen(host) + 2);
 	if (!conn->sspitarget)
 	{
-		printfPQExpBuffer(&conn->errorMessage, libpq_gettext("out of memory\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("out of memory\n"));
 		return STATUS_ERROR;
 	}
 	sprintf(conn->sspitarget, "%s/%s", conn->krbsrvname, host);
@@ -425,15 +429,15 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 	if (conn->channel_binding[0] == 'r' &&	/* require */
 		!conn->ssl_in_use)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("channel binding required, but SSL not in use\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("channel binding required, but SSL not in use\n"));
 		goto error;
 	}
 
 	if (conn->sasl_state)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("duplicate SASL authentication request\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("duplicate SASL authentication request\n"));
 		goto error;
 	}
 
@@ -448,8 +452,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 	{
 		if (pqGets(&mechanism_buf, conn))
 		{
-			printfPQExpBuffer(&conn->errorMessage,
-							  "fe_sendauth: invalid authentication request from server: invalid list of authentication mechanisms\n");
+			appendPQExpBufferStr(&conn->errorMessage,
+								 "fe_sendauth: invalid authentication request from server: invalid list of authentication mechanisms\n");
 			goto error;
 		}
 		if (PQExpBufferDataBroken(mechanism_buf))
@@ -488,8 +492,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 				 */
 				if (conn->channel_binding[0] == 'r')	/* require */
 				{
-					printfPQExpBuffer(&conn->errorMessage,
-									  libpq_gettext("channel binding is required, but client does not support it\n"));
+					appendPQExpBufferStr(&conn->errorMessage,
+										 libpq_gettext("channel binding is required, but client does not support it\n"));
 					goto error;
 				}
 #endif
@@ -505,8 +509,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 				 * the client and server supported it. The SCRAM exchange
 				 * checks for that, to prevent downgrade attacks.
 				 */
-				printfPQExpBuffer(&conn->errorMessage,
-								  libpq_gettext("server offered SCRAM-SHA-256-PLUS authentication over a non-SSL connection\n"));
+				appendPQExpBufferStr(&conn->errorMessage,
+									 libpq_gettext("server offered SCRAM-SHA-256-PLUS authentication over a non-SSL connection\n"));
 				goto error;
 			}
 		}
@@ -517,16 +521,16 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 
 	if (!selected_mechanism)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("none of the server's SASL authentication mechanisms are supported\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("none of the server's SASL authentication mechanisms are supported\n"));
 		goto error;
 	}
 
 	if (conn->channel_binding[0] == 'r' &&	/* require */
 		strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("channel binding is required, but server did not offer an authentication method that supports channel binding\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("channel binding is required, but server did not offer an authentication method that supports channel binding\n"));
 		goto error;
 	}
 
@@ -546,8 +550,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 		password = conn->pgpass;
 	if (password == NULL || password[0] == '\0')
 	{
-		printfPQExpBuffer(&conn->errorMessage,
-						  PQnoPasswordSupplied);
+		appendPQExpBufferStr(&conn->errorMessage,
+							 PQnoPasswordSupplied);
 		goto error;
 	}
 
@@ -607,8 +611,8 @@ oom_error:
 	termPQExpBuffer(&mechanism_buf);
 	if (initialresponse)
 		free(initialresponse);
-	printfPQExpBuffer(&conn->errorMessage,
-					  libpq_gettext("out of memory\n"));
+	appendPQExpBufferStr(&conn->errorMessage,
+						 libpq_gettext("out of memory\n"));
 	return STATUS_ERROR;
 }
 
@@ -631,7 +635,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
 	challenge = malloc(payloadlen + 1);
 	if (!challenge)
 	{
-		printfPQExpBuffer(&conn->errorMessage,
+		appendPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("out of memory allocating SASL buffer (%d)\n"),
 						  payloadlen);
 		return STATUS_ERROR;
@@ -656,8 +660,8 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
 		if (outputlen != 0)
 			free(output);
 
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("AuthenticationSASLFinal received from server, but SASL authentication was not completed\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("AuthenticationSASLFinal received from server, but SASL authentication was not completed\n"));
 		return STATUS_ERROR;
 	}
 	if (outputlen != 0)
@@ -726,15 +730,15 @@ pg_local_sendauth(PGconn *conn)
 	{
 		char		sebuf[PG_STRERROR_R_BUFLEN];
 
-		printfPQExpBuffer(&conn->errorMessage,
+		appendPQExpBuffer(&conn->errorMessage,
 						  "pg_local_sendauth: sendmsg: %s\n",
 						  strerror_r(errno, sebuf, sizeof(sebuf)));
 		return STATUS_ERROR;
 	}
 	return STATUS_OK;
 #else
-	printfPQExpBuffer(&conn->errorMessage,
-					  libpq_gettext("SCM_CRED authentication method not supported\n"));
+	appendPQExpBufferStr(&conn->errorMessage,
+						 libpq_gettext("SCM_CRED authentication method not supported\n"));
 	return STATUS_ERROR;
 #endif
 }
@@ -766,8 +770,8 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 				crypt_pwd = malloc(2 * (MD5_PASSWD_LEN + 1));
 				if (!crypt_pwd)
 				{
-					printfPQExpBuffer(&conn->errorMessage,
-									  libpq_gettext("out of memory\n"));
+					appendPQExpBufferStr(&conn->errorMessage,
+										 libpq_gettext("out of memory\n"));
 					return STATUS_ERROR;
 				}
 
@@ -832,14 +836,14 @@ check_expected_areq(AuthRequest areq, PGconn *conn)
 			case AUTH_REQ_OK:
 				if (!pg_fe_scram_channel_bound(conn->sasl_state))
 				{
-					printfPQExpBuffer(&conn->errorMessage,
-									  libpq_gettext("channel binding required, but server authenticated client without channel binding\n"));
+					appendPQExpBufferStr(&conn->errorMessage,
+										 libpq_gettext("channel binding required, but server authenticated client without channel binding\n"));
 					result = false;
 				}
 				break;
 			default:
-				printfPQExpBuffer(&conn->errorMessage,
-								  libpq_gettext("channel binding required but not supported by server's authentication request\n"));
+				appendPQExpBufferStr(&conn->errorMessage,
+									 libpq_gettext("channel binding required but not supported by server's authentication request\n"));
 				result = false;
 				break;
 		}
@@ -862,6 +866,8 @@ check_expected_areq(AuthRequest areq, PGconn *conn)
 int
 pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 {
+	int			oldmsglen;
+
 	if (!check_expected_areq(areq, conn))
 		return STATUS_ERROR;
 
@@ -871,13 +877,13 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 			break;
 
 		case AUTH_REQ_KRB4:
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("Kerberos 4 authentication not supported\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("Kerberos 4 authentication not supported\n"));
 			return STATUS_ERROR;
 
 		case AUTH_REQ_KRB5:
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("Kerberos 5 authentication not supported\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("Kerberos 5 authentication not supported\n"));
 			return STATUS_ERROR;
 
 #if defined(ENABLE_GSS) || defined(ENABLE_SSPI)
@@ -947,8 +953,8 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 			/* No GSSAPI *or* SSPI support */
 		case AUTH_REQ_GSS:
 		case AUTH_REQ_GSS_CONT:
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("GSSAPI authentication not supported\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("GSSAPI authentication not supported\n"));
 			return STATUS_ERROR;
 #endif							/* defined(ENABLE_GSS) || defined(ENABLE_SSPI) */
 
@@ -979,16 +985,16 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 			 */
 #if !defined(ENABLE_GSS)
 		case AUTH_REQ_SSPI:
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("SSPI authentication not supported\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("SSPI authentication not supported\n"));
 			return STATUS_ERROR;
 #endif							/* !define(ENABLE_GSS) */
 #endif							/* ENABLE_SSPI */
 
 
 		case AUTH_REQ_CRYPT:
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("Crypt authentication not supported\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("Crypt authentication not supported\n"));
 			return STATUS_ERROR;
 
 		case AUTH_REQ_MD5:
@@ -1002,14 +1008,14 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 					password = conn->pgpass;
 				if (password == NULL || password[0] == '\0')
 				{
-					printfPQExpBuffer(&conn->errorMessage,
-									  PQnoPasswordSupplied);
+					appendPQExpBufferStr(&conn->errorMessage,
+										 PQnoPasswordSupplied);
 					return STATUS_ERROR;
 				}
 				if (pg_password_sendauth(conn, password, areq) != STATUS_OK)
 				{
-					printfPQExpBuffer(&conn->errorMessage,
-									  "fe_sendauth: error sending password authentication\n");
+					appendPQExpBufferStr(&conn->errorMessage,
+										 "fe_sendauth: error sending password authentication\n");
 					return STATUS_ERROR;
 				}
 				break;
@@ -1032,17 +1038,18 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 		case AUTH_REQ_SASL_FIN:
 			if (conn->sasl_state == NULL)
 			{
-				printfPQExpBuffer(&conn->errorMessage,
-								  "fe_sendauth: invalid authentication request from server: AUTH_REQ_SASL_CONT without AUTH_REQ_SASL\n");
+				appendPQExpBufferStr(&conn->errorMessage,
+									 "fe_sendauth: invalid authentication request from server: AUTH_REQ_SASL_CONT without AUTH_REQ_SASL\n");
 				return STATUS_ERROR;
 			}
+			oldmsglen = conn->errorMessage.len;
 			if (pg_SASL_continue(conn, payloadlen,
 								 (areq == AUTH_REQ_SASL_FIN)) != STATUS_OK)
 			{
-				/* Use error message, if set already */
-				if (conn->errorMessage.len == 0)
-					printfPQExpBuffer(&conn->errorMessage,
-									  "fe_sendauth: error in SASL authentication\n");
+				/* Use this message if pg_SASL_continue didn't supply one */
+				if (conn->errorMessage.len == oldmsglen)
+					appendPQExpBufferStr(&conn->errorMessage,
+										 "fe_sendauth: error in SASL authentication\n");
 				return STATUS_ERROR;
 			}
 			break;
@@ -1053,7 +1060,7 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 			break;
 
 		default:
-			printfPQExpBuffer(&conn->errorMessage,
+			appendPQExpBuffer(&conn->errorMessage,
 							  libpq_gettext("authentication method %u not supported\n"), areq);
 			return STATUS_ERROR;
 	}
@@ -1067,7 +1074,7 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
  *
  * Returns a pointer to malloc'd space containing whatever name the user
  * has authenticated to the system.  If there is an error, return NULL,
- * and put a suitable error message in *errorMessage if that's not NULL.
+ * and append a suitable error message to *errorMessage if that's not NULL.
  */
 char *
 pg_fe_getauthname(PQExpBuffer errorMessage)
@@ -1100,7 +1107,7 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
 	if (GetUserName(username, &namesize))
 		name = username;
 	else if (errorMessage)
-		printfPQExpBuffer(errorMessage,
+		appendPQExpBuffer(errorMessage,
 						  libpq_gettext("user name lookup failure: error code %lu\n"),
 						  GetLastError());
 #else
@@ -1110,12 +1117,12 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
 	else if (errorMessage)
 	{
 		if (pwerr != 0)
-			printfPQExpBuffer(errorMessage,
+			appendPQExpBuffer(errorMessage,
 							  libpq_gettext("could not look up local user ID %d: %s\n"),
 							  (int) user_id,
 							  strerror_r(pwerr, pwdbuf, sizeof(pwdbuf)));
 		else
-			printfPQExpBuffer(errorMessage,
+			appendPQExpBuffer(errorMessage,
 							  libpq_gettext("local user with ID %d does not exist\n"),
 							  (int) user_id);
 	}
@@ -1125,8 +1132,8 @@ pg_fe_getauthname(PQExpBuffer errorMessage)
 	{
 		result = strdup(name);
 		if (result == NULL && errorMessage)
-			printfPQExpBuffer(errorMessage,
-							  libpq_gettext("out of memory\n"));
+			appendPQExpBufferStr(errorMessage,
+								 libpq_gettext("out of memory\n"));
 	}
 
 	pgunlock_thread();
@@ -1196,6 +1203,8 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
 	if (!conn)
 		return NULL;
 
+	resetPQExpBuffer(&conn->errorMessage);
+
 	/* If no algorithm was given, ask the server. */
 	if (algorithm == NULL)
 	{
@@ -1217,8 +1226,8 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
 		if (PQntuples(res) != 1 || PQnfields(res) != 1)
 		{
 			PQclear(res);
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("unexpected shape of result set returned for SHOW\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("unexpected shape of result set returned for SHOW\n"));
 			return NULL;
 		}
 		val = PQgetvalue(res, 0, 0);
@@ -1226,8 +1235,8 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
 		if (strlen(val) > MAX_ALGORITHM_NAME_LEN)
 		{
 			PQclear(res);
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("password_encryption value too long\n"));
+			appendPQExpBufferStr(&conn->errorMessage,
+								 libpq_gettext("password_encryption value too long\n"));
 			return NULL;
 		}
 		strcpy(algobuf, val);
@@ -1266,15 +1275,15 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
 	}
 	else
 	{
-		printfPQExpBuffer(&conn->errorMessage,
+		appendPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("unrecognized password encryption algorithm \"%s\"\n"),
 						  algorithm);
 		return NULL;
 	}
 
 	if (!crypt_pwd)
-		printfPQExpBuffer(&conn->errorMessage,
-						  libpq_gettext("out of memory\n"));
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("out of memory\n"));
 
 	return crypt_pwd;
 }