public inbox for [email protected]
help / color / mirror / Atom feedRe: Potential buffer overrun in spell.c's CheckAffix()
2+ messages / 1 participants
[nested] [flat]
* Re: Potential buffer overrun in spell.c's CheckAffix()
@ 2026-04-23 09:58 Andrey Borodin <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Andrey Borodin @ 2026-04-23 09:58 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL mailing lists <[email protected]>
> On 22 Apr 2026, at 18:44, Tom Lane <[email protected]> wrote:
>
> Andrey Borodin <[email protected]> writes:
>>> On 21 Apr 2026, at 22:32, Tom Lane <[email protected]> wrote:
>>> + /* protect against buffer overrun */
>>> + if (len < Affix->replen || len >= 2 * MAXNORMLEN ||
>>> + len - Affix->replen + findlen >= 2 * MAXNORMLEN)
>>> + return NULL;
>>> +
>>> strcpy(newword, word);
>>> strcpy(newword + len - Affix->replen, Affix->find);
>
>> Is there a reason for an asymmetric check "len >= 2 * MAXNORMLEN ||”?
>
> Yes. Because of that initial "strcpy(newword, word);", we do actually
> need "word" to fit in the output buffer, even if the final output
> string is shorter. The other path does not require that.
>
> I suppose we could replace the strcpy with
>
> memcpy(newword, word, len - Affix->replen);
>
> and then we would not need the "len >= 2 * MAXNORMLEN" test
> and both paths could share the same check. There's something
> to be said for that, though it would be changing the logic to
> a greater extent than just "add some safety checks".
The argument about not changing behavior in back branches is very convincing.
But IMO v2 of the patch is better. Also I think changes are correct.
>
>>> I chose to do this by silently truncating the input before it can
>>> overrun the buffer, using logic comparable to the existing logic in
>>> get_nextfield(). Certainly there's at least as good an argument for
>>> raising an error, but for now let's follow the existing precedent.
>
>> Is there a reason not to emit WARNING? The data is obviously suspicious…
>> Perhaps, there’s a reason, so maybe just document it then.
>
> I could agree with changing all of these cases (including the existing
> get_nextfield checks) to throw ERROR; but I don't think that's
> something to do in a back-patched bug fix.
Makes sense.
>
> Another thing I don't love, but wouldn't change in back branches,
> is the use of BUFSIZ for the string lengths. That's far more than
> necessary (why not just MAXNORMLEN?), causing these functions to eat
> much more stack space than they need. Also it seems like an
> unnecessary platform dependency. Maybe BUFSIZ is 8K everywhere,
> but I'm not sure of that.
On my machine (MacOS) BUFSIZ is 1Kb.
Yes, 40Kb in NIImportOOAffixes() is a lot. But is it important in grand scheme of
things? Minimum max_stack_depth is 100Kb, ought to be enough…
Perhaps, “replen" and “find" should not exceed MAXNORMLEN.
My limited understanding of affixes is not enough to confidently tell that
MAXNORMLEN is the limit. e.g. I see this:
* newword: output buffer (MUST be of length 2 * MAXNORMLEN)
So general rule “MAXNORMLEN is an upper bound everywhere” is not uphold.
I’m still under impression of understanding why NUM_MAX_ITEM_SIZ == 8 in
the nearby thread. Now I know a lot more about Roman numbers. Digging
deeper here might be a similar rabbit hole :)
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Potential buffer overrun in spell.c's CheckAffix()
@ 2026-04-30 07:56 Andrey Borodin <[email protected]>
parent: Andrey Borodin <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Andrey Borodin @ 2026-04-30 07:56 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: PostgreSQL mailing lists <[email protected]>
> On 23 Apr 2026, at 12:58, Andrey Borodin <[email protected]> wrote:
>
> Yes, 40Kb in NIImportOOAffixes() is a lot. But is it important in grand scheme of
> things? Minimum max_stack_depth is 100Kb, ought to be enough…
IsAffixFlagInUse(), addCompoundAffixFlagValue() and getCompoundAffixFlagValue()
also allocate 8Kb on stack...
Would it make sense to add check_stack_depth() into addCompoundAffixFlagValue()?
Other prominent allocators (NIImportOOAffixes(),NIImportAffixes()) call it anyway.
At least we will know if disaster is around the corner.
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-04-30 07:56 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-23 09:58 Re: Potential buffer overrun in spell.c's CheckAffix() Andrey Borodin <[email protected]>
2026-04-30 07:56 ` Andrey Borodin <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox