public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Andrey Borodin <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: Potential buffer overrun in spell.c's CheckAffix()
Date: Wed, 22 Apr 2026 09:44:40 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

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

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

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Potential buffer overrun in spell.c's CheckAffix()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox