From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrey Borodin <x4mmm@yandex-team.ru>
cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: Potential buffer overrun in spell.c's CheckAffix()
In-reply-to: <F6506DC9-7E95-4FA3-A9E3-37392E92463D@yandex-team.ru>
References: <641711.1776792744@sss.pgh.pa.us> <F6506DC9-7E95-4FA3-A9E3-37392E92463D@yandex-team.ru>
Comments: In-reply-to Andrey Borodin <x4mmm@yandex-team.ru>
	message dated "Wed, 22 Apr 2026 16:57:26 +0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <959932.1776865480.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Wed, 22 Apr 2026 09:44:40 -0400
Message-ID: <959933.1776865480@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/959933.1776865480%40sss.pgh.pa.us>
Precedence: bulk

Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> On 21 Apr 2026, at 22:32, Tom Lane <tgl@sss.pgh.pa.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".

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

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.

			regards, tom lane