Re: [HACKERS] Code quality issues in ICU patch

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
lengthper 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