Code quality issues in ICU patch

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Code quality issues in ICU patch
Date: 2017-06-23 16:31:40
Message-ID: 23278.1498235500@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2017-06-23 16:35:40 Re: Setting pd_lower in GIN metapage
Previous Message Sergey Burladyan 2017-06-23 15:23:12 Re: Broken hint bits (freeze)