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

> 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.

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