Tighten up make_libc_collator() and make_icu_collator().

Ensure that error paths within these functions do not leak a collator,
and return the result rather than using an out parameter. (Error paths
in the caller may still result in a leaked collator, which will be
addressed separately.)

In make_libc_collator(), if the first newlocale() succeeds and the
second one fails, close the first locale_t object.

The function make_icu_collator() doesn't have any external callers, so
change it to be static.

Discussion: https://postgr.es/m/54d20e812bd6c3e44c10eddcd757ec494ebf1803.camel@j-davis.com

diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index 5bef1b113a8b8a4b5b0d87ec671862a89fe3efee..8a7dde21398e78a0f20b72e820295f8ef892256c 100644 (file)
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1297,14 +1297,15 @@ report_newlocale_failure(const char *localename)
 }
 
 /*
- * Initialize the locale_t field.
+ * Create a locale_t with the given collation and ctype.
  *
- * The "C" and "POSIX" locales are not actually handled by libc, so set the
- * locale_t to zero in that case.
+ * The "C" and "POSIX" locales are not actually handled by libc, so return
+ * NULL.
+ *
+ * Ensure that no path leaks a locale_t.
  */
-static void
-make_libc_collator(const char *collate, const char *ctype,
-                                  pg_locale_t result)
+static locale_t
+make_libc_collator(const char *collate, const char *ctype)
 {
        locale_t        loc = 0;
 
@@ -1343,7 +1344,11 @@ make_libc_collator(const char *collate, const char *ctype,
                        errno = 0;
                        loc = newlocale(LC_CTYPE_MASK, ctype, loc1);
                        if (!loc)
+                       {
+                               if (loc1)
+                                       freelocale(loc1);
                                report_newlocale_failure(ctype);
+                       }
                }
                else
                        loc = loc1;
@@ -1360,60 +1365,78 @@ make_libc_collator(const char *collate, const char *ctype,
 #endif
        }
 
-       result->info.lt = loc;
+       return loc;
 }
 
-void
-make_icu_collator(const char *iculocstr,
-                                 const char *icurules,
-                                 struct pg_locale_struct *resultp)
-{
+/*
+ * Create a UCollator with the given locale string and rules.
+ *
+ * Ensure that no path leaks a UCollator.
+ */
 #ifdef USE_ICU
-       UCollator  *collator;
-
-       collator = pg_ucol_open(iculocstr);
-
-       /*
-        * If rules are specified, we extract the rules of the standard collation,
-        * add our own rules, and make a new collator with the combined rules.
-        */
-       if (icurules)
+static UCollator *
+make_icu_collator(const char *iculocstr, const char *icurules)
+{
+       if (!icurules)
+       {
+               /* simple case without rules */
+               return pg_ucol_open(iculocstr);
+       }
+       else
        {
-               const UChar *default_rules;
-               UChar      *agg_rules;
+               UCollator  *collator_std_rules;
+               UCollator  *collator_all_rules;
+               const UChar *std_rules;
                UChar      *my_rules;
-               UErrorCode      status;
+               UChar      *all_rules;
                int32_t         length;
+               int32_t         total;
+               UErrorCode      status;
 
-               default_rules = ucol_getRules(collator, &length);
+               /*
+                * If rules are specified, we extract the rules of the standard
+                * collation, add our own rules, and make a new collator with the
+                * combined rules.
+                */
                icu_to_uchar(&my_rules, icurules, strlen(icurules));
 
-               agg_rules = palloc_array(UChar, u_strlen(default_rules) + u_strlen(my_rules) + 1);
-               u_strcpy(agg_rules, default_rules);
-               u_strcat(agg_rules, my_rules);
+               collator_std_rules = pg_ucol_open(iculocstr);
 
-               ucol_close(collator);
+               std_rules = ucol_getRules(collator_std_rules, &length);
+
+               total = u_strlen(std_rules) + u_strlen(my_rules) + 1;
+
+               /* avoid leaking collator on OOM */
+               all_rules = palloc_extended(sizeof(UChar) * total, MCXT_ALLOC_NO_OOM);
+               if (!all_rules)
+               {
+                       ucol_close(collator_std_rules);
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_OUT_OF_MEMORY),
+                                        errmsg("out of memory")));
+               }
+
+               u_strcpy(all_rules, std_rules);
+               u_strcat(all_rules, my_rules);
+
+               ucol_close(collator_std_rules);
 
                status = U_ZERO_ERROR;
-               collator = ucol_openRules(agg_rules, u_strlen(agg_rules),
-                                                                 UCOL_DEFAULT, UCOL_DEFAULT_STRENGTH, NULL, &status);
+               collator_all_rules = ucol_openRules(all_rules, u_strlen(all_rules),
+                                                                                       UCOL_DEFAULT, UCOL_DEFAULT_STRENGTH,
+                                                                                       NULL, &status);
                if (U_FAILURE(status))
+               {
                        ereport(ERROR,
                                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                         errmsg("could not open collator for locale \"%s\" with rules \"%s\": %s",
                                                        iculocstr, icurules, u_errorName(status))));
-       }
+               }
 
-       /* We will leak this string if the caller errors later :-( */
-       resultp->info.icu.locale = MemoryContextStrdup(TopMemoryContext, iculocstr);
-       resultp->info.icu.ucol = collator;
-#else                                                  /* not USE_ICU */
-       /* could get here if a collation was created by a build with ICU */
-       ereport(ERROR,
-                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                        errmsg("ICU is not supported in this build")));
-#endif                                                 /* not USE_ICU */
+               return collator_all_rules;
+       }
 }
+#endif                                                 /* not USE_ICU */
 
 /*
  * Initialize default_locale with database locale settings.
@@ -1424,7 +1447,6 @@ init_database_collation(void)
        HeapTuple       tup;
        Form_pg_database dbform;
        Datum           datum;
-       bool            isnull;
 
        /* Fetch our pg_database row normally, via syscache */
        tup = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId));
@@ -1449,8 +1471,10 @@ init_database_collation(void)
        }
        else if (dbform->datlocprovider == COLLPROVIDER_ICU)
        {
+#ifdef USE_ICU
                char       *datlocale;
                char       *icurules;
+               bool            isnull;
 
                datum = SysCacheGetAttrNotNull(DATABASEOID, tup, Anum_pg_database_datlocale);
                datlocale = TextDatumGetCString(datum);
@@ -1464,15 +1488,20 @@ init_database_collation(void)
                else
                        icurules = NULL;
 
-               make_icu_collator(datlocale, icurules, &default_locale);
+               default_locale.info.icu.locale = MemoryContextStrdup(TopMemoryContext, datlocale);
+               default_locale.info.icu.ucol = make_icu_collator(datlocale, icurules);
+#else
+               /* could get here if a collation was created by a build with ICU */
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("ICU is not supported in this build")));
+#endif
        }
-       else
+       else if (dbform->datlocprovider == COLLPROVIDER_LIBC)
        {
                const char *datcollate;
                const char *datctype;
 
-               Assert(dbform->datlocprovider == COLLPROVIDER_LIBC);
-
                datum = SysCacheGetAttrNotNull(DATABASEOID, tup, Anum_pg_database_datcollate);
                datcollate = TextDatumGetCString(datum);
                datum = SysCacheGetAttrNotNull(DATABASEOID, tup, Anum_pg_database_datctype);
@@ -1483,8 +1512,12 @@ init_database_collation(void)
                default_locale.ctype_is_c = (strcmp(datctype, "C") == 0) ||
                        (strcmp(datctype, "POSIX") == 0);
 
-               make_libc_collator(datcollate, datctype, &default_locale);
+               default_locale.info.lt = make_libc_collator(datcollate, datctype);
        }
+       else
+               /* shouldn't happen */
+               PGLOCALE_SUPPORT_ERROR(dbform->datlocprovider);
+
 
        default_locale.provider = dbform->datlocprovider;
 
@@ -1557,25 +1590,9 @@ pg_newlocale_from_collation(Oid collid)
                        result.info.builtin.locale = MemoryContextStrdup(TopMemoryContext,
                                                                                                                         locstr);
                }
-               else if (collform->collprovider == COLLPROVIDER_LIBC)
-               {
-                       const char *collcollate;
-                       const char *collctype;
-
-                       datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
-                       collcollate = TextDatumGetCString(datum);
-                       datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collctype);
-                       collctype = TextDatumGetCString(datum);
-
-                       result.collate_is_c = (strcmp(collcollate, "C") == 0) ||
-                               (strcmp(collcollate, "POSIX") == 0);
-                       result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
-                               (strcmp(collctype, "POSIX") == 0);
-
-                       make_libc_collator(collcollate, collctype, &result);
-               }
                else if (collform->collprovider == COLLPROVIDER_ICU)
                {
+#ifdef USE_ICU
                        const char *iculocstr;
                        const char *icurules;
 
@@ -1591,8 +1608,35 @@ pg_newlocale_from_collation(Oid collid)
                        else
                                icurules = NULL;
 
-                       make_icu_collator(iculocstr, icurules, &result);
+                       result.info.icu.locale = MemoryContextStrdup(TopMemoryContext, iculocstr);
+                       result.info.icu.ucol = make_icu_collator(iculocstr, icurules);
+#else
+                       /* could get here if a collation was created by a build with ICU */
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                        errmsg("ICU is not supported in this build")));
+#endif
                }
+               else if (collform->collprovider == COLLPROVIDER_LIBC)
+               {
+                       const char *collcollate;
+                       const char *collctype;
+
+                       datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collcollate);
+                       collcollate = TextDatumGetCString(datum);
+                       datum = SysCacheGetAttrNotNull(COLLOID, tp, Anum_pg_collation_collctype);
+                       collctype = TextDatumGetCString(datum);
+
+                       result.collate_is_c = (strcmp(collcollate, "C") == 0) ||
+                               (strcmp(collcollate, "POSIX") == 0);
+                       result.ctype_is_c = (strcmp(collctype, "C") == 0) ||
+                               (strcmp(collctype, "POSIX") == 0);
+
+                       result.info.lt = make_libc_collator(collcollate, collctype);
+               }
+               else
+                       /* shouldn't happen */
+                       PGLOCALE_SUPPORT_ERROR(collform->collprovider);
 
                datum = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion,
                                                                &isnull);
@@ -2500,6 +2544,8 @@ builtin_validate_locale(int encoding, const char *locale)
 /*
  * Wrapper around ucol_open() to handle API differences for older ICU
  * versions.
+ *
+ * Ensure that no path leaks a UCollator.
  */
 static UCollator *
 pg_ucol_open(const char *loc_str)

diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index faae868bfccff7b24518d646748b593dfbbcb462..c2d95411e0a53eaf75852ec939ac8df61cc2f159 100644 (file)
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -104,10 +104,6 @@ struct pg_locale_struct
 
 typedef struct pg_locale_struct *pg_locale_t;
 
-extern void make_icu_collator(const char *iculocstr,
-                                                         const char *icurules,
-                                                         struct pg_locale_struct *resultp);
-
 extern void init_database_collation(void);
 extern pg_locale_t pg_newlocale_from_collation(Oid collid);