From 7b86c2ac9563ffd9b870cfd73a769431b7922e81 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 20 Sep 2017 13:52:36 -0400 Subject: [PATCH] Improve dubious memory management in pg_newlocale_from_collation(). pg_newlocale_from_collation() used malloc() and strdup() directly, which is generally not per backend coding style, and it didn't bother to check for failure results, but would just SIGSEGV instead. Also, if one of the numerous error checks in the middle of the function failed, the already-allocated memory would be leaked permanently. Admittedly, it's not a lot of memory, but it could build up if this function were called repeatedly for a bad collation. The first two problems are easily cured by palloc'ing in TopMemoryContext instead of calling libc directly. We can fairly easily dodge the leakage problem for the struct pg_locale_struct by filling in a temporary variable and allocating permanent storage only once we reach the bottom of the function. It's harder to get rid of the potential leakage for ICU's copy of the collcollate string, but at least that's only allocated after most of the error checks; so live with that aspect. Back-patch to v10 where this code came in, with one or another of the ICU patches. --- src/backend/utils/adt/pg_locale.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 13e7625b20..3d3d8aa4b6 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1292,7 +1292,8 @@ pg_newlocale_from_collation(Oid collid) Form_pg_collation collform; const char *collcollate; const char *collctype pg_attribute_unused(); - pg_locale_t result; + struct pg_locale_struct result; + pg_locale_t resultp; Datum collversion; bool isnull; @@ -1304,9 +1305,9 @@ pg_newlocale_from_collation(Oid collid) collcollate = NameStr(collform->collcollate); collctype = NameStr(collform->collctype); - result = malloc(sizeof(*result)); - memset(result, 0, sizeof(*result)); - result->provider = collform->collprovider; + /* We'll fill in the result struct locally before allocating memory */ + memset(&result, 0, sizeof(result)); + result.provider = collform->collprovider; if (collform->collprovider == COLLPROVIDER_LIBC) { @@ -1353,7 +1354,7 @@ pg_newlocale_from_collation(Oid collid) #endif } - result->info.lt = loc; + result.info.lt = loc; #else /* not HAVE_LOCALE_T */ /* platform that doesn't support locale_t */ ereport(ERROR, @@ -1379,8 +1380,10 @@ pg_newlocale_from_collation(Oid collid) (errmsg("could not open collator for locale \"%s\": %s", collcollate, u_errorName(status)))); - result->info.icu.locale = strdup(collcollate); - result->info.icu.ucol = collator; + /* We will leak this string if we get an error below :-( */ + result.info.icu.locale = MemoryContextStrdup(TopMemoryContext, + collcollate); + result.info.icu.ucol = collator; #else /* not USE_ICU */ /* could get here if a collation was created by a build with ICU */ ereport(ERROR, @@ -1427,7 +1430,11 @@ pg_newlocale_from_collation(Oid collid) ReleaseSysCache(tp); - cache_entry->locale = result; + /* We'll keep the pg_locale_t structures in TopMemoryContext */ + resultp = MemoryContextAlloc(TopMemoryContext, sizeof(*resultp)); + *resultp = result; + + cache_entry->locale = resultp; } return cache_entry->locale; -- 2.40.0