Re: [HACKERS] Code quality issues in ICU patch
On 6/30/17 08:13, Peter Eisentraut wrote: > On 6/24/17 11:51, Tom Lane wrote: >> Ah, I was about to suggest the same thing, but I was coming at it from >> the standpoint of not requiring buffers several times larger than >> necessary, which could in itself cause avoidable palloc failures. >> >> I was going to suggest a small variant actually: run the conversion >> function an extra time only if the string is long enough to make the >> space consumption interesting, say > > I had thought about something like that, too, but my concern is that we > then have double the code paths to test. I have run some performance > tests and I couldn't detect any differences between the variants. So > unless someone has any other insights, I think I'll go with the proposed > patch by tomorrow. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
On 6/24/17 11:51, Tom Lane wrote: > Ah, I was about to suggest the same thing, but I was coming at it from > the standpoint of not requiring buffers several times larger than > necessary, which could in itself cause avoidable palloc failures. > > I was going to suggest a small variant actually: run the conversion > function an extra time only if the string is long enough to make the > space consumption interesting, say I had thought about something like that, too, but my concern is that we then have double the code paths to test. I have run some performance tests and I couldn't detect any differences between the variants. So unless someone has any other insights, I think I'll go with the proposed patch by tomorrow. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
On Sun, Jun 25, 2017 at 09:28:51PM -0700, Noah Misch wrote: > On Sat, Jun 24, 2017 at 10:03:25AM -0400, Peter Eisentraut wrote: > > On 6/23/17 12:31, Tom Lane wrote: > > > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > > > touchingly naive about integer overflow hazards in buffer size > > > calculations. I call particular attention to this bit in > > > icu_from_uchar(): > > > > > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, > > > ucnv_getMaxCharSize(icu_converter)); > > > > > > The ICU man pages say that that macro is defined as > > > > > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) > > > (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > > > > > which means that getting this to overflow (resulting in > > > probably-exploitable memory overruns) would be about as hard as taking > > > candy from a baby. > > > > Here is a patch that should address this. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
On Sat, Jun 24, 2017 at 10:03:25AM -0400, Peter Eisentraut wrote: > On 6/23/17 12:31, Tom Lane wrote: > > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > > touchingly naive about integer overflow hazards in buffer size > > calculations. I call particular attention to this bit in > > icu_from_uchar(): > > > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, > > ucnv_getMaxCharSize(icu_converter)); > > > > The ICU man pages say that that macro is defined as > > > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) > > (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > > > which means that getting this to overflow (resulting in > > probably-exploitable memory overruns) would be about as hard as taking > > candy from a baby. > > Here is a patch that should address this. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
Peter Eisentraut writes: > On 6/23/17 12:31, Tom Lane wrote: >> icu_to_uchar() and icu_from_uchar(), and perhaps other places, are >> touchingly naive about integer overflow hazards in buffer size >> calculations. > Here is a patch that should address this. Ah, I was about to suggest the same thing, but I was coming at it from the standpoint of not requiring buffers several times larger than necessary, which could in itself cause avoidable palloc failures. I was going to suggest a small variant actually: run the conversion function an extra time only if the string is long enough to make the space consumption interesting, say if (nbytes < 1024) { /* if it's short, feel free to waste a bit of space */ len_uchar = 2 * nbytes + 1; /* max length per docs */ } else { /* calculate exact space needed */ status = U_ZERO_ERROR; len_uchar = ucnv_toUChars(icu_converter, NULL, 0, buff, nbytes, &status); if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) ... } *buff_uchar = palloc(len_uchar * sizeof(**buff_uchar)); In this way the extra cycles would seldom be paid in practice. > (I don't think the overruns were exploitable. You'd just get a buffer > overflow error from the ucnv_* function.) Hm, good point. But we might still hit avoidable failures with strings that are a significant fraction of 1GB. 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
Re: [HACKERS] Code quality issues in ICU patch
On 6/23/17 12:31, Tom Lane wrote: > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > touchingly naive about integer overflow hazards in buffer size > calculations. I call particular attention to this bit in > icu_from_uchar(): > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, > ucnv_getMaxCharSize(icu_converter)); > > The ICU man pages say that that macro is defined as > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) > (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > which means that getting this to overflow (resulting in > probably-exploitable memory overruns) would be about as hard as taking > candy from a baby. Here is a patch that should address this. (I don't think the overruns were exploitable. You'd just get a buffer overflow error from the ucnv_* function.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From c909db0741cb7cdf0b6249e063c7ad78cbf93819 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sat, 24 Jun 2017 09:39:24 -0400 Subject: [PATCH] Refine memory allocation in ICU conversions The simple calculations done to estimate the size of the output buffers for ucnv_fromUChars() and ucnv_toUChars() could overflow int32_t for large strings. To avoid that, go the long way and run the function first without an output buffer to get the correct output buffer size requirement. --- src/backend/utils/adt/pg_locale.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 0f5ec954c3..5478e39ea7 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1506,14 +1506,22 @@ icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes) init_icu_converter(); - len_uchar = 2 * nbytes + 1; /* max length per docs */ - *buff_uchar = palloc(len_uchar * sizeof(**buff_uchar)); status = U_ZERO_ERROR; - len_uchar = ucnv_toUChars(icu_converter, *buff_uchar, len_uchar, + len_uchar = ucnv_toUChars(icu_converter, NULL, 0, + buff, nbytes, &status); + if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) + ereport(ERROR, + (errmsg("ucnv_toUChars failed: %s", u_errorName(status; + + *buff_uchar = palloc((len_uchar + 1) * sizeof(**buff_uchar)); + + status = U_ZERO_ERROR; + len_uchar = ucnv_toUChars(icu_converter, *buff_uchar, len_uchar + 1, buff, nbytes, &status); if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_toUChars failed: %s", u_errorName(status; + return len_uchar; } @@ -1536,14 +1544,22 @@ icu_from_uchar(char **result, const UChar *buff_uchar, int32_t len_uchar) init_icu_converter(); - len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); + status = U_ZERO_ERROR; + len_result = ucnv_fromUChars(icu_converter, NULL, 0, +buff_uchar, len_uchar, &status); + if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR) + ereport(ERROR, + (errmsg("ucnv_fromUChars failed: %s", u_errorName(status; + *result = palloc(len_result + 1); + status = U_ZERO_ERROR; len_result = ucnv_fromUChars(icu_converter, *result, len_result + 1, buff_uchar, len_uchar, &status); if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_fromUChars failed: %s", u_errorName(status; + return len_result; } #endif /* USE_ICU */ -- 2.13.1 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Code quality issues in ICU patch
On Fri, Jun 23, 2017 at 12:31:40PM -0400, Tom Lane wrote: > icu_to_uchar() and icu_from_uchar(), and perhaps other places, are > touchingly naive about integer overflow hazards in buffer size > calculations. I call particular attention to this bit in > icu_from_uchar(): > > len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, > ucnv_getMaxCharSize(icu_converter)); > > The ICU man pages say that that macro is defined as > > #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) > (((int32_t)(length)+10)*(int32_t)(maxCharSize)) > > which means that getting this to overflow (resulting in > probably-exploitable memory overruns) would be about as hard as > taking candy from a baby. So it kicks off really loud and persistent alarms, and isn't as easy as you thought, even taking this into account? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Code quality issues in ICU patch
icu_to_uchar() and icu_from_uchar(), and perhaps other places, are touchingly naive about integer overflow hazards in buffer size calculations. I call particular attention to this bit in icu_from_uchar(): len_result = UCNV_GET_MAX_BYTES_FOR_STRING(len_uchar, ucnv_getMaxCharSize(icu_converter)); The ICU man pages say that that macro is defined as #define UCNV_GET_MAX_BYTES_FOR_STRING(length, maxCharSize) (((int32_t)(length)+10)*(int32_t)(maxCharSize)) which means that getting this to overflow (resulting in probably-exploitable memory overruns) would be about as hard as taking candy from a baby. I also notice that the general approach to handling ICU-reported error conditions is like if (U_FAILURE(status)) ereport(ERROR, (errmsg("ucnv_fromUChars failed: %s", u_errorName(status; This lacks an errcode() setting, which is contrary to project policy, and the error message violates our message style guidelines. I don't particularly feel like fixing these things myself, but somebody needs to; the overflow issues in particular are stop-ship security hazards. 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