Fix PGLC_localeconv() to handle errors better.

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: <CAB7nPqRMbGqa_mesopcn4MPyTs34eqtVEK7ELYxvvV=oqS00YA@mail.gmail.com>
9 years ago · a4930e7ca2
parent 4324ade9a6
commit a4930e7ca2
1 changed files with 164 additions and 52 deletions
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@ -382,51 +382,91 @@ 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 = pg_any_to_server(str, strlen(str), encoding);
+	pstr = pg_any_to_server(*str, strlen(*str), encoding);
+	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")));
+
+	/* replace old string */
+	free(*str);
+	*str = mstr;

-	return mstr;
+	pfree(pstr);
 }


@ -440,13 +480,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
@ -462,6 +499,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)
@ -477,7 +528,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
 	 *
@ -487,7 +538,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
@ -499,6 +550,8 @@ 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
@ -506,11 +559,11 @@ PGLC_localeconv(void)
 	/* 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 */
@ -520,40 +573,36 @@ 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
@ -561,11 +610,74 @@ PGLC_localeconv(void)
 	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;
 }