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

Hi.

Em dom., 2 de nov. de 2025 às 13:09, Bryan Green <dbryan(dot)green(at)gmail(dot)com>
escreveu:

> 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...
>
Thanks for taking a look.

I really appreciate your suggestion for the comment,
but I believe it makes it harder to understand.

best regards,
Ranier Vilela