Re: BUG #19595: Three memory-safety defects in src/backend/tsearch/spell.c (dictionary loader), PG 18.3

Hi,

One more problem in the same file. It is not one of the three in the
original report - those were all on the affix-rule side (CompoundAffix,
the flag buffer, the AF alias table), while this one is in the compound
flag table - and it is older than all of them, the code being from 9.6.
So I'm posting here rather than opening a new report.

CompoundAffixFlag holds a flag in a union whose member is chosen by the
flag mode the affix file's FLAG line declares, and NIImportOOAffixes()
converts each COMPOUNDFLAG / ONLYINCOMPOUND / ... flag as soon as it reads
the line, using the mode in effect at that point. FLAG may appear anywhere
in the file, so flags read before and after it can disagree about which
member holds the value. cmpcmdflag() takes the mode from its first
argument alone and applies it to both, so it can read an integer as a char
pointer and hand it to strcmp().

Three ways this shows up. Starting with the one that doesn't crash, which
I think matters most: take the shipped hunspell_sample_num dictionary and
move its FLAG line below the compound options, changing nothing else.

COMPOUNDFLAG 101
ONLYINCOMPOUND 102
FLAG num

SELECT ts_lexize('withflagfirst', 'footballklubber');
{footballklubber,foot,ball,klubber,football,klubber}
SELECT ts_lexize('withflaglast', 'footballklubber');
{footballklubber}

The entries are stored as strings while the lookup key is built with the
final mode, so the numeric comparison compares a pointer's low half against
a number, never matches, and the compound flag is never found. No error,
no warning, and suffix handling still works ('books' -> {book}), so the
dictionary looks healthy. Compound splitting is simply gone, along with
every lexeme it would have produced: a search for 'ball' stops finding the
document, and a GIN index built this way never contained those lexemes.

The two crashing shapes:

COMPOUNDFLAG A / FLAG num / COMPOUNDBEGIN 1
-> heterogeneous array, dies in the sort
SIGSEGV __strcmp_evex <- cmpcmdflag spell.c:226 <- pg_qsort
<- NIImportOOAffixes spell.c:1306

FLAG num / COMPOUNDFLAG 101 / ONLYINCOMPOUND 102 / FLAG long
-> array consistent, but the lookup key uses the final mode
SIGSEGV <- cmpcmdflag spell.c:226 <- bsearch
<- getCompoundAffixFlagValue spell.c:1152
<- makeCompoundFlags spell.c:1652 <- mkSPNode spell.c:1725

With --enable-cassert the Assert() in cmpcmdflag() fires first. All three
reproduce on 8646214ee20 on both build types, and the code involved has not
changed in a long time. Reachability is as for the defects fixed by
330a72052cd - you must be able to place a file in $SHAREDIR/tsearch_data -
and every dictionary I know of, including all four shipped samples, puts
FLAG first, which is presumably why this went unreported for ten years.

0001 keeps the flags as strings while the file is read and converts them
once it has been read in full, when the mode is final. That removes all
three symptoms, since the array and the lookup key then use the same
representation.

It also makes the position of FLAG irrelevant, which is what the rest of
the parser already does: AF, SFX and PFX flags are parsed in a second pass
and so always use the final mode. PG is in fact already more permissive
than hunspell(5), which says "If the affix file contains the FLAG parameter,
define it before the AF definitions" - move FLAG below the AF lines of
hunspell_sample_long and PG still gets it right. So FLAG is currently
file-scoped for AF/SFX/PFX and line-scoped for the eight compound options;
0001 makes the latter agree. Erroring out instead would be about six
lines, but that would leave us stricter than Hunspell for compound options
while staying looser for AF, which seems hard to justify.

Note the old ispell format reaches this code from NIImportAffixes() and
returns without entering NIImportOOAffixes(), so it needs the conversion
too - I missed that at first and ispell_sample promptly crashed the
regression run.

0002 is optional cleanup. With 0001 in place every entry agrees
with Conf->flagMode, so the per-entry copy of the mode is redundant; it
exists only because of the comment on the field, "we don't have a
bsearch_arg version, so, copy FlagMode", which stopped being true when
bsearch_arg() moved to src/port in bfa2cee7841. Taking the mode through
qsort_arg()/bsearch_arg() lets the field and the Assert() go. No functional
change, and 0001 does not depend on it.

Regards,
Ewan Young

On Mon, Aug 3, 2026 at 4:11 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alexander Lakhin <exclusion(at)gmail(dot)com> writes:
> > I'm not sure it's directly related to this bug report, but maybe you'd
> > like to fix one more memory-safety defect in tsearch in passing...
>
> Hmph. Not sure I'd call that "memory safety", but yeah, this bit
> isn't being careful about having a valid intermediate state of the
> data structure. Thanks for the report!
>
> regards, tom lane
>
>

--
Regards,
Ewan Young