Re: Fix overflow at return wchar2char (src/backend/utils/adt/pg_locale.c)

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
Cc: Daniel Gustafsson <daniel(at)yesql(dot)se>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Fix overflow at return wchar2char (src/backend/utils/adt/pg_locale.c)
Date: 2020-09-14 18:07:58
Message-ID: 1126118.1600106878@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> writes:
> Em seg., 14 de set. de 2020 às 10:53, Daniel Gustafsson <daniel(at)yesql(dot)se>
> escreveu:
>> If the objection is that an unsigned var is tested with <= 0, then
>> changing the
>> semantics of the function seems a rather drastic solution:

> But if wchar2char must be follow wcstombs_l API.
> wchar2char all calls must be:

> result = wchar2char();
> if (result == 0 || result == (size_t)-1) {

> See at lowerstr_with_len (src/backend/tsearch/ts_locale.c):

Actually, lowerstr_with_len is perfectly fine. It's coercing the
size_t result to int, so (size_t) -1 becomes integer -1 and its
error check for wlen < 0 is correct. It might have a problem if
the coercion to int could overflow, but that cannot happen because
of our restrictions on the size of a palloc'd chunk.

There are some other call sites that are failing to check at all,
which is not so good. But changing the function's API to be both
nonstandard and ambiguous (because a zero result is a perfectly valid
case) doesn't help fix that.

I concur with Daniel that none of the changes shown here are
worthwhile improvements. It's not illegal to test an unsigned
variable for "x <= 0".

regards, tom lane

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ranier Vilela 2020-09-14 18:11:01 Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch
Previous Message Peter Geoghegan 2020-09-14 17:55:56 Re: Fix for parallel BTree initialization bug