Thread: Fix overflow at return wchar2char (src/backend/utils/adt/pg_locale.c)
Hi,
1. wchar2char has a mistake when checking the return of WideCharToMultiByte call.
result variable is unsigned, therefore, cannot be less than zero, returning -1 is not an option.
2. strftime or strftime_win32, return cannot be less than zero.
3. If strftime or strftime_win32, fails, why not abort the loop?
regards,
Ranier Vilela
Attachment
Re: Fix overflow at return wchar2char (src/backend/utils/adt/pg_locale.c)
From
Daniel Gustafsson
Date:
> On 14 Sep 2020, at 14:41, Ranier Vilela <ranier.vf@gmail.com> wrote:
> 1. wchar2char has a mistake when checking the return of WideCharToMultiByte call.
> result variable is unsigned, therefore, cannot be less than zero, returning -1 is not an option.
If the objection is that an unsigned var is tested with <= 0, then changing the
semantics of the function seems a rather drastic solution:
/* A zero return is failure */
- if (result <= 0)
- result = -1;
+ if (result == 0)
+ return 0;
The comment for wchar2char explicitly state "This has the same API as the
standard wcstombs_l() function;", and man wcstombs_l shows:
RETURN VALUES
The wcstombs() function returns the number of bytes converted (not
including any terminating null), if successful; otherwise, it returns
(size_t)-1.
It can of course be argued that the check should be "result == 0" as result is
of type size_t. The original commit introducing this in 2007, 654dcfb9e4b6,
had an integer return variable, so it's just a carry-over from there. Will
changing that buy us anything, except possibly silence a static analyzer?
> 2. strftime or strftime_win32, return cannot be less than zero.
>
> 3. If strftime or strftime_win32, fails, why not abort the loop?
This recently changed in 7ad1cd31bfc, and the commit message along with the
comment above the code implies that an error is unlikely:,
* MAX_L10N_DATA is sufficient buffer space for every known locale, and
* POSIX defines no strftime() errors. (Buffer space exhaustion is not an
* error.)
..so it's probably a case of not optimizing for never-happens-scenarios: The
fact that strftimefail will trigger elog and not ereport is an additional clue
that an error is unlikely.
cheers ./daniel
Em seg., 14 de set. de 2020 às 10:53, Daniel Gustafsson <daniel@yesql.se> escreveu:
> On 14 Sep 2020, at 14:41, Ranier Vilela <ranier.vf@gmail.com> wrote:
> 1. wchar2char has a mistake when checking the return of WideCharToMultiByte call.
> result variable is unsigned, therefore, cannot be less than zero, returning -1 is not an option.
If the objection is that an unsigned var is tested with <= 0, then changing the
semantics of the function seems a rather drastic solution:
/* A zero return is failure */
- if (result <= 0)
- result = -1;
+ if (result == 0)
+ return 0;
The comment for wchar2char explicitly state "This has the same API as the
standard wcstombs_l() function;", and man wcstombs_l shows:
RETURN VALUES
The wcstombs() function returns the number of bytes converted (not
including any terminating null), if successful; otherwise, it returns
(size_t)-1.
I'm not agree that must, follow wcstombs_l API.
And there is already a precedent in returning zero.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):
wlen = char2wchar(wstr, len + 1, str, len, mylocale); // fail with -1
Assert((void) 0); // Release mode, Assert(wlen <= len);
wlen = 18446744073709551615;len = pg_database_encoding_max_length() * 18446744073709551615 + 1; // len is int, Windows LLP64 is 32 bits.
out = (char *) palloc(len);
> 2. strftime or strftime_win32, return cannot be less than zero.
>
> 3. If strftime or strftime_win32, fails, why not abort the loop?
This recently changed in 7ad1cd31bfc, and the commit message along with the
comment above the code implies that an error is unlikely:,
* MAX_L10N_DATA is sufficient buffer space for every known locale, and
* POSIX defines no strftime() errors. (Buffer space exhaustion is not an
* error.)
..so it's probably a case of not optimizing for never-happens-scenarios: The
fact that strftimefail will trigger elog and not ereport is an additional clue
that an error is unlikely.
The cost of the test, It has been paid, then the break is free.
And testing unsigned if it is less than zero, it is useless,
it just gets in the way of the compiler.
Ranier Vilela
Ranier Vilela <ranier.vf@gmail.com> writes:
> Em seg., 14 de set. de 2020 às 10:53, Daniel Gustafsson <daniel@yesql.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