Fix PGLC_localeconv() to handle errors better.

tglsfdc · tglsfdc · commit cd5dfea32d4a · 2016-11-21T18:21:56.000-05:00
The code was intentionally not very careful about leaking strdup'd
strings in case of an error.  That was forgivable probably, but it
also failed to notice strdup() failures, which could lead to subsequent
null-pointer-dereference crashes, since many callers unsurprisingly
didn't check for null pointers in the struct lconv fields.  An even
worse problem is that it could throw error while we were setlocale'd
to a non-C locale, causing unwanted behavior in subsequent libc calls.

Rewrite to ensure that we cannot throw elog(ERROR) until after we've
restored the previous locale settings, or at least attempted to.
(I'm sorely tempted to make restore failure be a FATAL error, but
will refrain for the moment.)  Having done that, it's not much more
work to ensure that we clean up strdup'd storage on the way out, too.

This code is substantially the same in all supported branches, so
back-patch all the way.

Michael Paquier and Tom Lane

Discussion: &lt;CAB7nPqRMbGqa_mesopcn4MPyTs34eqtVEK7ELYxvvV=oqS00YA@mail.gmail.com&gt;
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
@@ -358,53 +358,93 @@ assign_locale_messages(const char *newval, void *extra)
 
 /*
  * Frees the malloced content of a struct lconv.  (But not the struct
- * itself.)
+ * itself.)  It's important that this not throw elog(ERROR).
  */
 static void
 free_struct_lconv(struct lconv * s)
 {
-	if (s->currency_symbol)
-		free(s->currency_symbol);
 	if (s->decimal_point)
 		free(s->decimal_point);
-	if (s->grouping)
-		free(s->grouping);
 	if (s->thousands_sep)
 		free(s->thousands_sep);
+	if (s->grouping)
+		free(s->grouping);
 	if (s->int_curr_symbol)
 		free(s->int_curr_symbol);
+	if (s->currency_symbol)
+		free(s->currency_symbol);
 	if (s->mon_decimal_point)
 		free(s->mon_decimal_point);
-	if (s->mon_grouping)
-		free(s->mon_grouping);
 	if (s->mon_thousands_sep)
 		free(s->mon_thousands_sep);
-	if (s->negative_sign)
-		free(s->negative_sign);
+	if (s->mon_grouping)
+		free(s->mon_grouping);
 	if (s->positive_sign)
 		free(s->positive_sign);
+	if (s->negative_sign)
+		free(s->negative_sign);
+}
+
+/*
+ * Check that all fields of a struct lconv (or at least, the ones we care
+ * about) are non-NULL.  The field list must match free_struct_lconv().
+ */
+static bool
+struct_lconv_is_valid(struct lconv * s)
+{
+	if (s->decimal_point == NULL)
+		return false;
+	if (s->thousands_sep == NULL)
+		return false;
+	if (s->grouping == NULL)
+		return false;
+	if (s->int_curr_symbol == NULL)
+		return false;
+	if (s->currency_symbol == NULL)
+		return false;
+	if (s->mon_decimal_point == NULL)
+		return false;
+	if (s->mon_thousands_sep == NULL)
+		return false;
+	if (s->mon_grouping == NULL)
+		return false;
+	if (s->positive_sign == NULL)
+		return false;
+	if (s->negative_sign == NULL)
+		return false;
+	return true;
 }
 
 
 /*
- * Return a strdup'ed string converted from the specified encoding to the
+ * Convert the strdup'd string at *str from the specified encoding to the
  * database encoding.
  */
-static char *
-db_encoding_strdup(int encoding, const char *str)
+static void
+db_encoding_convert(int encoding, char **str)
 {
 	char	   *pstr;
 	char	   *mstr;
 
 	/* convert the string to the database encoding */
 	pstr = (char *) pg_do_encoding_conversion(
-										  (unsigned char *) str, strlen(str),
+										  (unsigned char *) *str, strlen(*str),
 											encoding, GetDatabaseEncoding());
+	if (pstr == *str)
+		return;					/* no conversion happened */
+
+	/* need it malloc'd not palloc'd */
 	mstr = strdup(pstr);
-	if (pstr != str)
-		pfree(pstr);
+	if (mstr == NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_OUT_OF_MEMORY),
+				 errmsg("out of memory")));
 
-	return mstr;
+	/* replace old string */
+	free(*str);
+	*str = mstr;
+
+	pfree(pstr);
 }
 
 
@@ -418,13 +458,10 @@ PGLC_localeconv(void)
 	static struct lconv CurrentLocaleConv;
 	static bool CurrentLocaleConvAllocated = false;
 	struct lconv *extlconv;
+	struct lconv worklconv;
+	bool		trouble = false;
 	char	   *save_lc_monetary;
 	char	   *save_lc_numeric;
-	char	   *decimal_point;
-	char	   *grouping;
-	char	   *thousands_sep;
-	int			encoding;
-
 #ifdef WIN32
 	char	   *save_lc_ctype;
 #endif
@@ -440,6 +477,20 @@ PGLC_localeconv(void)
 		CurrentLocaleConvAllocated = false;
 	}
 
+	/*
+	 * This is tricky because we really don't want to risk throwing error
+	 * while the locale is set to other than our usual settings.  Therefore,
+	 * the process is: collect the usual settings, set locale to special
+	 * setting, copy relevant data into worklconv using strdup(), restore
+	 * normal settings, convert data to desired encoding, and finally stash
+	 * the collected data in CurrentLocaleConv.  This makes it safe if we
+	 * throw an error during encoding conversion or run out of memory anywhere
+	 * in the process.  All data pointed to by struct lconv members is
+	 * allocated with strdup, to avoid premature elog(ERROR) and to allow
+	 * using a single cleanup routine.
+	 */
+	memset(&worklconv, 0, sizeof(worklconv));
+
 	/* Save user's values of monetary and numeric locales */
 	save_lc_monetary = setlocale(LC_MONETARY, NULL);
 	if (save_lc_monetary)
@@ -455,7 +506,7 @@ PGLC_localeconv(void)
 	 * Ideally, monetary and numeric local symbols could be returned in any
 	 * server encoding.  Unfortunately, the WIN32 API does not allow
 	 * setlocale() to return values in a codepage/CTYPE that uses more than
-	 * two bytes per character, like UTF-8:
+	 * two bytes per character, such as UTF-8:
 	 *
 	 * http://msdn.microsoft.com/en-us/library/x99tb11d.aspx
 	 *
@@ -465,7 +516,7 @@ PGLC_localeconv(void)
 	 *
 	 * Therefore, we set LC_CTYPE to match LC_NUMERIC or LC_MONETARY (which
 	 * cannot be UTF8), call localeconv(), and then convert from the
-	 * numeric/monitary LC_CTYPE to the server encoding.  One example use of
+	 * numeric/monetary LC_CTYPE to the server encoding.  One example use of
 	 * this is for the Euro symbol.
 	 *
 	 * Perhaps someday we will use GetLocaleInfoW() which returns values in
@@ -477,18 +528,20 @@ PGLC_localeconv(void)
 	if (save_lc_ctype)
 		save_lc_ctype = pstrdup(save_lc_ctype);
 
+	/* Here begins the critical section where we must not throw error */
+
 	/* use numeric to set the ctype */
 	setlocale(LC_CTYPE, locale_numeric);
 #endif
 
 	/* Get formatting information for numeric */
 	setlocale(LC_NUMERIC, locale_numeric);
 	extlconv = localeconv();
-	encoding = pg_get_encoding_from_locale(locale_numeric, true);
 
-	decimal_point = db_encoding_strdup(encoding, extlconv->decimal_point);
-	thousands_sep = db_encoding_strdup(encoding, extlconv->thousands_sep);
-	grouping = strdup(extlconv->grouping);
+	/* Must copy data now in case setlocale() overwrites it */
+	worklconv.decimal_point = strdup(extlconv->decimal_point);
+	worklconv.thousands_sep = strdup(extlconv->thousands_sep);
+	worklconv.grouping = strdup(extlconv->grouping);
 
 #ifdef WIN32
 	/* use monetary to set the ctype */
@@ -498,52 +551,111 @@ PGLC_localeconv(void)
 	/* Get formatting information for monetary */
 	setlocale(LC_MONETARY, locale_monetary);
 	extlconv = localeconv();
-	encoding = pg_get_encoding_from_locale(locale_monetary, true);
 
-	/*
-	 * Must copy all values since restoring internal settings may overwrite
-	 * localeconv()'s results.  Note that if we were to fail within this
-	 * sequence before reaching "CurrentLocaleConvAllocated = true", we could
-	 * leak some memory --- but not much, so it's not worth agonizing over.
-	 */
-	CurrentLocaleConv = *extlconv;
-	CurrentLocaleConv.decimal_point = decimal_point;
-	CurrentLocaleConv.grouping = grouping;
-	CurrentLocaleConv.thousands_sep = thousands_sep;
-	CurrentLocaleConv.int_curr_symbol = db_encoding_strdup(encoding, extlconv->int_curr_symbol);
-	CurrentLocaleConv.currency_symbol = db_encoding_strdup(encoding, extlconv->currency_symbol);
-	CurrentLocaleConv.mon_decimal_point = db_encoding_strdup(encoding, extlconv->mon_decimal_point);
-	CurrentLocaleConv.mon_grouping = strdup(extlconv->mon_grouping);
-	CurrentLocaleConv.mon_thousands_sep = db_encoding_strdup(encoding, extlconv->mon_thousands_sep);
-	CurrentLocaleConv.negative_sign = db_encoding_strdup(encoding, extlconv->negative_sign);
-	CurrentLocaleConv.positive_sign = db_encoding_strdup(encoding, extlconv->positive_sign);
-	CurrentLocaleConvAllocated = true;
+	/* Must copy data now in case setlocale() overwrites it */
+	worklconv.int_curr_symbol = strdup(extlconv->int_curr_symbol);
+	worklconv.currency_symbol = strdup(extlconv->currency_symbol);
+	worklconv.mon_decimal_point = strdup(extlconv->mon_decimal_point);
+	worklconv.mon_thousands_sep = strdup(extlconv->mon_thousands_sep);
+	worklconv.mon_grouping = strdup(extlconv->mon_grouping);
+	worklconv.positive_sign = strdup(extlconv->positive_sign);
+	worklconv.negative_sign = strdup(extlconv->negative_sign);
+	/* Copy scalar fields as well */
+	worklconv.int_frac_digits = extlconv->int_frac_digits;
+	worklconv.frac_digits = extlconv->frac_digits;
+	worklconv.p_cs_precedes = extlconv->p_cs_precedes;
+	worklconv.p_sep_by_space = extlconv->p_sep_by_space;
+	worklconv.n_cs_precedes = extlconv->n_cs_precedes;
+	worklconv.n_sep_by_space = extlconv->n_sep_by_space;
+	worklconv.p_sign_posn = extlconv->p_sign_posn;
+	worklconv.n_sign_posn = extlconv->n_sign_posn;
 
 	/* Try to restore internal settings */
 	if (save_lc_monetary)
 	{
 		if (!setlocale(LC_MONETARY, save_lc_monetary))
-			elog(WARNING, "failed to restore old locale");
-		pfree(save_lc_monetary);
+			trouble = true;
 	}
 
 	if (save_lc_numeric)
 	{
 		if (!setlocale(LC_NUMERIC, save_lc_numeric))
-			elog(WARNING, "failed to restore old locale");
-		pfree(save_lc_numeric);
+			trouble = true;
 	}
 
 #ifdef WIN32
 	/* Try to restore internal ctype settings */
 	if (save_lc_ctype)
 	{
 		if (!setlocale(LC_CTYPE, save_lc_ctype))
-			elog(WARNING, "failed to restore old locale");
-		pfree(save_lc_ctype);
+			trouble = true;
 	}
 #endif
 
+	/*
+	 * At this point we've done our best to clean up, and can call functions
+	 * that might possibly throw errors with a clean conscience.  But let's
+	 * make sure we don't leak any already-strdup'd fields in worklconv.
+	 */
+	PG_TRY();
+	{
+		int			encoding;
+
+		/*
+		 * Report it if we failed to restore anything.  Perhaps this should be
+		 * FATAL, rather than continuing with bad locale settings?
+		 */
+		if (trouble)
+			elog(WARNING, "failed to restore old locale");
+
+		/* Release the pstrdup'd locale names */
+		if (save_lc_monetary)
+			pfree(save_lc_monetary);
+		if (save_lc_numeric)
+			pfree(save_lc_numeric);
+#ifdef WIN32
+		if (save_lc_ctype)
+			pfree(save_lc_ctype);
+#endif
+
+		/* If any of the preceding strdup calls failed, complain now. */
+		if (!struct_lconv_is_valid(&worklconv))
+			ereport(ERROR,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory")));
+
+		/*
+		 * Now we must perform encoding conversion from whatever's associated
+		 * with the locale into the database encoding.
+		 */
+		encoding = pg_get_encoding_from_locale(locale_numeric, true);
+
+		db_encoding_convert(encoding, &worklconv.decimal_point);
+		db_encoding_convert(encoding, &worklconv.thousands_sep);
+		/* grouping is not text and does not require conversion */
+
+		encoding = pg_get_encoding_from_locale(locale_monetary, true);
+
+		db_encoding_convert(encoding, &worklconv.int_curr_symbol);
+		db_encoding_convert(encoding, &worklconv.currency_symbol);
+		db_encoding_convert(encoding, &worklconv.mon_decimal_point);
+		db_encoding_convert(encoding, &worklconv.mon_thousands_sep);
+		/* mon_grouping is not text and does not require conversion */
+		db_encoding_convert(encoding, &worklconv.positive_sign);
+		db_encoding_convert(encoding, &worklconv.negative_sign);
+	}
+	PG_CATCH();
+	{
+		free_struct_lconv(&worklconv);
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+
+	/*
+	 * Everything is good, so save the results.
+	 */
+	CurrentLocaleConv = worklconv;
+	CurrentLocaleConvAllocated = true;
 	CurrentLocaleConvValid = true;
 	return &CurrentLocaleConv;
 }
