Re: Potential buffer overrun in spell.c's CheckAffix()

> On 22 Apr 2026, at 18:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Andrey Borodin <x4mmm(at)yandex-team(dot)ru> writes:
>>> On 21 Apr 2026, at 22:32, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.