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

Em seg., 14 de set. de 2020 às 10:53, Daniel Gustafsson <daniel(at)yesql(dot)se>
escreveu:

> > On 14 Sep 2020, at 14:41, Ranier Vilela <ranier(dot)vf(at)gmail(dot)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.

regards,
Ranier Vilela