Improve OOM handling in pg_locale.c
Hi all,
This is a follow-up of
/messages/by-id/11202.1472597262@sss.pgh.pa.us
where we are looking at improving OOM handling in the code. In short,
report an ERROR appropriately instead of crashing. As mentioned in
this message, pg_locale.c is trickier to handle because we had better
not call elog() in a code path where the backend's locale are not set
up appropriately. Attached is a patch aimed at fixing that, doing the
following:
- Copy into a temporary struct lconv the results from the call of
localeconv() as those can be overwritten when restoring back the
locales with setlocale().
- Use db_encoding_strdup to encode that correctly.
- Switch back to the backend locales
- Check for any strdup calls that returned NULL and elog()
- If no error, fill in CurrentLocaleConv and return back to caller.
I am attaching that to the next CF.
Thanks,
--
Michael
Attachments:
pglocale-oom.patchtext/plain; charset=US-ASCII; name=pglocale-oom.patchDownload
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index a818023..e4fd0f3 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -440,6 +440,7 @@ PGLC_localeconv(void)
static struct lconv CurrentLocaleConv;
static bool CurrentLocaleConvAllocated = false;
struct lconv *extlconv;
+ struct lconv tmplconv;
char *save_lc_monetary;
char *save_lc_numeric;
char *decimal_point;
@@ -523,23 +524,22 @@ PGLC_localeconv(void)
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.
+ * First copy all values into a temporary variable since restoring internal
+ * settings may overwrite localeconv()'s results. Note that elog() cannot
+ * be called as the backend's locale settings prevail and they are not
+ * restored yet.
*/
- 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;
+ tmplconv = *extlconv;
+ tmplconv.decimal_point = decimal_point;
+ tmplconv.grouping = grouping;
+ tmplconv.thousands_sep = thousands_sep;
+ tmplconv.int_curr_symbol = db_encoding_strdup(encoding, extlconv->int_curr_symbol);
+ tmplconv.currency_symbol = db_encoding_strdup(encoding, extlconv->currency_symbol);
+ tmplconv.mon_decimal_point = db_encoding_strdup(encoding, extlconv->mon_decimal_point);
+ tmplconv.mon_grouping = strdup(extlconv->mon_grouping);
+ tmplconv.mon_thousands_sep = db_encoding_strdup(encoding, extlconv->mon_thousands_sep);
+ tmplconv.negative_sign = db_encoding_strdup(encoding, extlconv->negative_sign);
+ tmplconv.positive_sign = db_encoding_strdup(encoding, extlconv->positive_sign);
/* Try to restore internal settings */
if (save_lc_monetary)
@@ -566,6 +566,31 @@ PGLC_localeconv(void)
}
#endif
+ /*
+ * Now that locales are set back to normal check for any NULL fields and
+ * report an out-of-memory error in consequence if any.
+ */
+ if (tmplconv.decimal_point == NULL ||
+ tmplconv.grouping == NULL ||
+ tmplconv.thousands_sep == NULL ||
+ tmplconv.int_curr_symbol == NULL ||
+ tmplconv.currency_symbol == NULL ||
+ tmplconv.mon_decimal_point == NULL ||
+ tmplconv.mon_grouping == NULL ||
+ tmplconv.mon_thousands_sep == NULL ||
+ tmplconv.negative_sign == NULL ||
+ tmplconv.positive_sign == NULL)
+ {
+ free_struct_lconv(&tmplconv);
+ elog(ERROR, "out of memory");
+ }
+
+ /*
+ * Everything is good, so save the result.
+ */
+ CurrentLocaleConv = tmplconv;
+
+ CurrentLocaleConvAllocated = true;
CurrentLocaleConvValid = true;
return &CurrentLocaleConv;
}
Hi Mithun,
This is a gentle reminder.
you assigned as reviewer to the current patch in the 11-2016 commitfest.
But you haven't shared your review yet. Can you please try to share your
views
about the patch. This will help us in smoother operation of commitfest.
Please Ignore if you already shared your review.
Regards,
Hari Babu
Fujitsu Australia
On Thu, Oct 13, 2016 at 1:40 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
I am attaching that to the next CF.
I have tested this patch. Now we error out as OOM instead of crash.
------------------------------------------------
postgres=# SELECT '12.34'::money;
ERROR: out of memory
LINE 1: SELECT '12.34'::money;
------------------------------------------------
One thing which you might need to reconsider is removal of memory leak
comments. There is still a leak if there is an error while encoding in
db_encoding_strdup.
Unless you want to catch those error with an TRY();....CATCH(); and then
free the mem.
- * 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.
Rest all LGTM.
--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com
Mithun Cy <mithun.cy@enterprisedb.com> writes:
On Thu, Oct 13, 2016 at 1:40 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:I am attaching that to the next CF.
One thing which you might need to reconsider is removal of memory leak
comments. There is still a leak if there is an error while encoding in
db_encoding_strdup.
Unless you want to catch those error with an TRY();....CATCH(); and then
free the mem.
I could have lived with leaving the leak there, but really this wasn't
fixing the worst problem with the code: if it did throw an error out of
the middle of that sequence, it would leave the process setlocale'd to
some other locale than we want. That could lead to unwanted behavior
in printf and other functions. And this isn't all that unlikely: an
encoding conversion failure is definitely possible if you have a locale
selected that's not compatible with the database encoding.
I whacked the patch around enough so that we didn't do anything except
libc calls between setting and restoring the locale. At that point it
was just a matter of adding a TRY block to be able to say that we
didn't leak any strdup'd strings, so I figured "might as well".
Pushed with those changes.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 22, 2016 at 8:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I could have lived with leaving the leak there, but really this wasn't
fixing the worst problem with the code: if it did throw an error out of
the middle of that sequence, it would leave the process setlocale'd to
some other locale than we want. That could lead to unwanted behavior
in printf and other functions. And this isn't all that unlikely: an
encoding conversion failure is definitely possible if you have a locale
selected that's not compatible with the database encoding.I whacked the patch around enough so that we didn't do anything except
libc calls between setting and restoring the locale. At that point it
was just a matter of adding a TRY block to be able to say that we
didn't leak any strdup'd strings, so I figured "might as well".Pushed with those changes.
Thanks. The changes you have done look good to me at short sight.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers