From ab0b59ad3b2f0e1c984aeb375953b7a1596bf4d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philip=20Dub=C3=A9?= Date: Thu, 9 Apr 2020 00:29:19 +0000 Subject: [PATCH] GetConnParams: Set runtimeParamStart before setting keywords/values to avoid out of bounds access --- .../connection/connection_configuration.c | 15 +++++++-------- .../connection/connection_management.c | 19 ++++++++++++++----- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/backend/distributed/connection/connection_configuration.c b/src/backend/distributed/connection/connection_configuration.c index 8e21fcee0..0e461b36e 100644 --- a/src/backend/distributed/connection/connection_configuration.c +++ b/src/backend/distributed/connection/connection_configuration.c @@ -261,6 +261,13 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values, GetDatabaseEncodingName() }; + /* + * remember where global/GUC params end and runtime ones start, all entries after this + * point should be allocated in context and will be freed upon + * FreeConnParamsHashEntryFields + */ + *runtimeParamStart = ConnParams.size; + /* * Declare local params for readability; * @@ -279,7 +286,6 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values, /* auth keywords will begin after global and runtime ones are appended */ Index authParamsIdx = ConnParams.size + lengthof(runtimeKeywords); - if (ConnParams.size + lengthof(runtimeKeywords) >= ConnParams.maxSize) { /* hopefully this error is only seen by developers */ @@ -297,13 +303,6 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values, connValues[paramIndex] = ConnParams.values[paramIndex]; } - /* - * remember where global/GUC params end and runtime ones start, all entries after this - * point should be allocated in context and will be freed upon - * FreeConnParamsHashEntryFields - */ - *runtimeParamStart = ConnParams.size; - /* second step: begin after global params and copy runtime params into our context */ for (Index runtimeParamIndex = 0; runtimeParamIndex < lengthof(runtimeKeywords); diff --git a/src/backend/distributed/connection/connection_management.c b/src/backend/distributed/connection/connection_management.c index 9c5bd17a5..f9b13f8ce 100644 --- a/src/backend/distributed/connection/connection_management.c +++ b/src/backend/distributed/connection/connection_management.c @@ -995,6 +995,16 @@ StartConnectionEstablishment(ConnectionHashKey *key) ConnParamsHashEntry *entry = hash_search(ConnParamsHash, key, HASH_ENTER, &found); if (!found || !entry->isValid) { + if (!found) + { + /* + * Zero out entry, but not the key part. + * Avoids leaving invalid pointers in hash table if GetConnParam throws with MemoryContextAllocZero. + */ + memset(((char *) entry) + sizeof(ConnectionHashKey), 0, + sizeof(ConnParamsHashEntry) - sizeof(ConnectionHashKey)); + } + /* avoid leaking memory in the keys and values arrays */ if (found && !entry->isValid) { @@ -1043,9 +1053,6 @@ StartConnectionEstablishment(ConnectionHashKey *key) static void FreeConnParamsHashEntryFields(ConnParamsHashEntry *entry) { - char **keyword = NULL; - char **value = NULL; - /* * if there was a memory error during the initialization of ConnParamHashEntry in * GetConnParams the keywords or values might not have been initialized completely. @@ -1058,7 +1065,7 @@ FreeConnParamsHashEntryFields(ConnParamsHashEntry *entry) if (entry->keywords != NULL) { - keyword = &entry->keywords[entry->runtimeParamStart]; + char **keyword = &entry->keywords[entry->runtimeParamStart]; while (*keyword != NULL) { pfree(*keyword); @@ -1070,7 +1077,7 @@ FreeConnParamsHashEntryFields(ConnParamsHashEntry *entry) if (entry->values != NULL) { - value = &entry->values[entry->runtimeParamStart]; + char **value = &entry->values[entry->runtimeParamStart]; while (*value != NULL) { pfree(*value); @@ -1079,6 +1086,8 @@ FreeConnParamsHashEntryFields(ConnParamsHashEntry *entry) pfree(entry->values); entry->values = NULL; } + + entry->runtimeParamStart = 0; }