Re: [HACKERS] Code quality issues in ICU patch

[Peter Eisentraut]

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

[Peter Eisentraut]

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

[Noah Misch]

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

[Noah Misch]

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

[Tom Lane]

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, );
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

[Peter Eisentraut]

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, 
);
+   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, 
);
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, );
+   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, );
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

[David Fetter]

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

[Tom Lane]

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