From 67c4dac89bebce8163a04439bea30c6887300e66 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Mon, 18 Sep 2017 21:26:36 +0300 Subject: [PATCH] Properly copy and trim the error messages that come from pg_conn When a NULL connection is provided to PQerrorMessage(), the returned error message is a static text. Modifying that static text, which doesn't necessarly be in a writeable memory, is dangreous and might cause a segfault. --- src/backend/distributed/commands/multi_copy.c | 13 ++----- .../distributed/connection/remote_commands.c | 36 +++++++++++++------ .../executor/multi_client_executor.c | 2 +- .../distributed/master/master_citus_tools.c | 7 +++- src/include/distributed/remote_commands.h | 1 + .../regress/expected/multi_router_planner.out | 2 -- src/test/regress/output/multi_copy.source | 7 ---- 7 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/backend/distributed/commands/multi_copy.c b/src/backend/distributed/commands/multi_copy.c index 0128c1695..c6c46de80 100644 --- a/src/backend/distributed/commands/multi_copy.c +++ b/src/backend/distributed/commands/multi_copy.c @@ -1143,17 +1143,8 @@ ReportCopyError(MultiConnection *connection, PGresult *result) } else { - /* probably a connection problem, get the message from the connection */ - char *lastNewlineIndex = NULL; - - remoteMessage = PQerrorMessage(connection->pgConn); - lastNewlineIndex = strrchr(remoteMessage, '\n'); - - /* trim trailing newline, if any */ - if (lastNewlineIndex != NULL) - { - *lastNewlineIndex = '\0'; - } + /* trim the trailing characters */ + remoteMessage = pchomp(PQerrorMessage(connection->pgConn)); ereport(ERROR, (errcode(ERRCODE_IO_ERROR), errmsg("failed to complete COPY on %s:%d", connection->hostname, diff --git a/src/backend/distributed/connection/remote_commands.c b/src/backend/distributed/connection/remote_commands.c index 050b8eee2..5710ef66d 100644 --- a/src/backend/distributed/connection/remote_commands.c +++ b/src/backend/distributed/connection/remote_commands.c @@ -17,6 +17,7 @@ #include "lib/stringinfo.h" #include "miscadmin.h" #include "storage/latch.h" +#include "utils/palloc.h" /* GUC, determining whether statements sent to remote nodes are logged */ @@ -116,7 +117,7 @@ ReportConnectionError(MultiConnection *connection, int elevel) int nodePort = connection->port; ereport(elevel, (errmsg("connection error: %s:%d", nodeName, nodePort), - errdetail("%s", PQerrorMessage(connection->pgConn)))); + errdetail("%s", pchomp(PQerrorMessage(connection->pgConn))))); } @@ -154,16 +155,7 @@ ReportResultError(MultiConnection *connection, PGresult *result, int elevel) */ if (messagePrimary == NULL) { - char *lastNewlineIndex = NULL; - - messagePrimary = PQerrorMessage(connection->pgConn); - lastNewlineIndex = strrchr(messagePrimary, '\n'); - - /* trim trailing newline, if any */ - if (lastNewlineIndex != NULL) - { - *lastNewlineIndex = '\0'; - } + messagePrimary = pchomp(PQerrorMessage(connection->pgConn)); } ereport(elevel, (errcode(sqlState), errmsg("%s", messagePrimary), @@ -182,6 +174,28 @@ ReportResultError(MultiConnection *connection, PGresult *result, int elevel) } +/* *INDENT-OFF* */ +#if (PG_VERSION_NUM < 100000) + +/* + * Make copy of string with all trailing newline characters removed. + */ +char * +pchomp(const char *in) +{ + size_t n; + + n = strlen(in); + while (n > 0 && in[n - 1] == '\n') + n--; + return pnstrdup(in, n); +} + +#endif + +/* *INDENT-ON* */ + + /* * LogRemoteCommand logs commands send to remote nodes if * citus.log_remote_commands wants us to do so. diff --git a/src/backend/distributed/executor/multi_client_executor.c b/src/backend/distributed/executor/multi_client_executor.c index ab71b7ae5..d7fa91deb 100644 --- a/src/backend/distributed/executor/multi_client_executor.c +++ b/src/backend/distributed/executor/multi_client_executor.c @@ -313,7 +313,7 @@ MultiClientSendQuery(int32 connectionId, const char *query) querySent = PQsendQuery(connection->pgConn, query); if (querySent == 0) { - char *errorMessage = PQerrorMessage(connection->pgConn); + char *errorMessage = pchomp(PQerrorMessage(connection->pgConn)); ereport(WARNING, (errmsg("could not send remote query \"%s\"", query), errdetail("Client error: %s", errorMessage))); diff --git a/src/backend/distributed/master/master_citus_tools.c b/src/backend/distributed/master/master_citus_tools.c index 946ec16cf..ccce8a620 100644 --- a/src/backend/distributed/master/master_citus_tools.c +++ b/src/backend/distributed/master/master_citus_tools.c @@ -439,7 +439,12 @@ StoreErrorMessage(PGconn *connection, StringInfo queryResultString) char *errorMessage = PQerrorMessage(connection); if (errorMessage != NULL) { - char *firstNewlineIndex = strchr(errorMessage, '\n'); + char *firstNewlineIndex = NULL; + + /* copy the error message to a writable memory */ + errorMessage = pnstrdup(errorMessage, strlen(errorMessage)); + + firstNewlineIndex = strchr(errorMessage, '\n'); /* trim the error message at the line break */ if (firstNewlineIndex != NULL) diff --git a/src/include/distributed/remote_commands.h b/src/include/distributed/remote_commands.h index 3799d9d97..f3bcbe847 100644 --- a/src/include/distributed/remote_commands.h +++ b/src/include/distributed/remote_commands.h @@ -32,6 +32,7 @@ extern bool SqlStateMatchesCategory(char *sqlStateString, int category); extern void ReportConnectionError(MultiConnection *connection, int elevel); extern void ReportResultError(MultiConnection *connection, struct pg_result *result, int elevel); +extern char * pchomp(const char *in); extern void LogRemoteCommand(MultiConnection *connection, const char *command); /* wrappers around libpq functions, with command logging support */ diff --git a/src/test/regress/expected/multi_router_planner.out b/src/test/regress/expected/multi_router_planner.out index 369ee2f32..6d557a6bc 100644 --- a/src/test/regress/expected/multi_router_planner.out +++ b/src/test/regress/expected/multi_router_planner.out @@ -2163,7 +2163,6 @@ BEGIN; INSERT INTO failure_test VALUES (1, 1); WARNING: connection error: localhost:57638 DETAIL: no connection to the server - SELECT shardid, shardstate, nodename, nodeport FROM pg_dist_shard_placement WHERE shardid IN ( SELECT shardid FROM pg_dist_shard @@ -2182,7 +2181,6 @@ ROLLBACK; INSERT INTO failure_test VALUES (2, 1); WARNING: connection error: localhost:57638 DETAIL: no connection to the server - SELECT shardid, shardstate, nodename, nodeport FROM pg_dist_shard_placement WHERE shardid IN ( SELECT shardid FROM pg_dist_shard diff --git a/src/test/regress/output/multi_copy.source b/src/test/regress/output/multi_copy.source index 3a68f0f21..5924c5f4e 100644 --- a/src/test/regress/output/multi_copy.source +++ b/src/test/regress/output/multi_copy.source @@ -878,19 +878,15 @@ ALTER USER test_user WITH nologin; COPY numbers_hash FROM STDIN WITH (FORMAT 'csv'); WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 1: "1,1" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 2: "2,2" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 3: "3,3" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash, line 6: "6,6" -- verify shards in the first worker as marked invalid SELECT shardid, shardstate, nodename, nodeport @@ -912,7 +908,6 @@ SELECT shardid, shardstate, nodename, nodeport COPY numbers_reference FROM STDIN WITH (FORMAT 'csv'); ERROR: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_reference, line 1: "3,1" -- verify shards for reference table are still valid SELECT shardid, shardstate, nodename, nodeport @@ -930,11 +925,9 @@ SELECT shardid, shardstate, nodename, nodeport COPY numbers_hash_other FROM STDIN WITH (FORMAT 'csv'); WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash_other, line 1: "1,1" WARNING: connection error: localhost:57637 DETAIL: FATAL: role "test_user" is not permitted to log in - CONTEXT: COPY numbers_hash_other, line 1: "1,1" ERROR: could not connect to any active placements CONTEXT: COPY numbers_hash_other, line 1: "1,1"