Re: Avoid overflow (src/backend/utils/adt/formatting.c)

On 11/2/2025 10:09 AM, Bryan Green wrote:
> On 11/2/2025 8:59 AM, Ranier Vilela wrote:
>> Hi.
>>
>> Per Coverity.
>>
>> Coverity raised the follow report:
>>
>> CID 1642824: (#1 of 1): Overflowed constant (INTEGER_OVERFLOW)
>> 37. overflow_const: Expression pattern_len - 1ULL, where pattern_len is 
>> known to be equal to 0, underflows the type of pattern_len - 1ULL, which 
>> is type unsigned long long.
>>
>> This is because the function *pg_mbstrlen* can return zero.
>> And the comment is clear, *just in case there are MB chars*.
>>
>> patch attached.
>>
>> best regards,
>> Ranier Vilela
> 
> Thanks for finding and patching this.  I think you're absolutely
> right that there's a latent bug here, and your fix is appropriate.
> However, I wanted to share some thoughts on why this has probably
> never been reported in the wild, and why we should apply the patch
> anyway.
> 
> The crash requires a specific and rather unlikely combination of
> circumstances:
> 
> 1. You need a locale where lconv->thousands_sep is present but
>    empty.  Most locales either provide a real separator (",", ".",
>    or " ") or return NULL (in which case NUM_prepare_locale's
>    fallback logic provides a default).  The comments mention
>    "broken glibc pt_BR" as an example, but even that's been fixed
>    in most glibc versions for years now.
> 
> 2. You need a format string containing 'G', which requests
>    insertion of the locale's thousands separator.
> 
> Here's the thing: if your locale has no thousands separator, why
> would you write a format string asking for one?  Consider:
> 
>     -- In a locale with thousands_sep = ","
>     SELECT to_char(1234567.89, '9G999G999.99');
>     -- Result: "1,234,567.89"
> 
>     -- In a locale with empty thousands_sep
>     SELECT to_char(1234567.89, '9G999G999.99');
>     -- Result: "1234567.89"  (the G's insert nothing)
> 
> If you're working in a locale that doesn't have a thousands
> separator, you're most likely culturally aware of that fact,
> and you simply wouldn't include 'G' in your format strings.
> It would be like writing code to format something your locale
> doesn't have a concept of.
> 
> So I think the bug has survived this long because:
>   a) Very few locales have truly empty thousands_sep strings
>   b) Users of those locales naturally avoid the 'G' format code
>   c) Even accidental use would require the right fill-mode
>      settings
> 
> That said, in my opinion we should definitely apply your patch,
> for several reasons:
> 
> First, it's undefined behavior.  Backing up the pointer via
> "ptr += -1" when ptr is at the start of allocated memory is a
> serious bug, regardless of how unlikely the trigger condition is.
> We don't want that lurking in the codebase.
> 
> Second, even if this hasn't been triggered in 20+ years, it's
> exactly the kind of thing that AFL or similar tools might find.
> Better to fix it before we get a CVE filing.
> 
> Third, it's a trivial fix.  The performance impact is nil, and it
> makes the code more obviously correct.  There's really no
> downside.
> 
> I'd suggest one minor adjustment to your patch: add a comment
> explaining why we're checking for empty pattern, since it's
> non-obvious:
> 
>     /*
>      * Guard against empty separator (could happen with some
>      * locales)
>      */
>     if (pattern_len > 0)
>     {
>         ...
>     }
> 
> That'll help future maintainers understand this isn't just
> defensive programming paranoia.
> 
> Good catch on finding this. We'll have to see if others agree...
> 
> Bryan Green
I did a bit more analysis to double check myself and realized I missed
an important check:

NUM_prepare_locale() has a safety check:

    if (lconv->thousands_sep && *lconv->thousands_sep)
        Np->L_thousands_sep = lconv->thousands_sep;
    else if (strcmp(Np->decimal, ",") != 0)
        Np->L_thousands_sep = ",";
    else
        Np->L_thousands_sep = ".";

The "*lconv->thousands_sep" test specifically prevents empty
strings from being used, so Np->L_thousands_sep can never be empty in
production.

The patch is still trivial and it probably should still be applied.

Bryan Green