Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)

The following bug has been logged on the website:

Bug reference: 15150
Logged by: Alexander Lakhin
Email address: exclusion(at)gmail(dot)com
PostgreSQL version: 10.3
Operating system: Debian-8
Description:

When trying to installcheck hunspell_nl_nl
(https://github.com/postgrespro/hunspell_dicts)
(see
https://github.com/postgrespro/hunspell_dicts/blob/master/hunspell_nl_nl/sql/hunspell_nl_nl.sql)
under valgrind, I get the following diagnostics:

==00:01:05:53.421 20772== Conditional jump or move depends on uninitialised
value(s)
==00:01:05:53.422 20772== at 0x4EA2C6: NISortAffixes (spell.c:1966)
==00:01:05:53.423 20772== by 0x4E5AA5: dispell_init (dict_ispell.c:90)
==00:01:05:53.425 20772== by 0x5E83F1: OidFunctionCall1Coll
(fmgr.c:1332)
==00:01:05:53.426 20772== by 0x36427C: verify_dictoptions.part.2
(tsearchcmds.c:399)
==00:01:05:53.426 20772== by 0x365ED2: verify_dictoptions
(tsearchcmds.c:477)
==00:01:05:53.427 20772== by 0x365ED2: DefineTSDictionary
(tsearchcmds.c:460)
==00:01:05:53.427 20772== by 0x4DE511: ProcessUtilitySlow.isra.5
(utility.c:1293)
==00:01:05:53.427 20772== by 0x4DCC70: standard_ProcessUtility
(utility.c:944)
==00:01:05:53.427 20772== by 0x7334815: pgss_ProcessUtility
(pg_stat_statements.c:1062)
==00:01:05:53.427 20772== by 0x7FB5DE4: pathman_process_utility_hook
(hooks.c:946)
==00:01:05:53.427 20772== by 0x320E99: execute_sql_string
(extension.c:763)
==00:01:05:53.427 20772== by 0x320E99: execute_extension_script.isra.7
(extension.c:924)
==00:01:05:53.427 20772== by 0x32187B: CreateExtensionInternal
(extension.c:1529)
==00:01:05:53.427 20772== by 0x321DD7: CreateExtension
(extension.c:1718)

It looks that the following condition in NISortAffixes(IspellDict *Conf)
uses uninitialised ptr->issuffix:

if (ptr == Conf->CompoundAffix ||
ptr->issuffix != (ptr - 1)->issuffix ||

Hello,

> Bug reference: 15150
> Logged by: Alexander Lakhin
> Email address: exclusion(at)gmail(dot)com
> PostgreSQL version: 10.3
> Operating system: Debian-8
> Description:
>
> It looks that the following condition in NISortAffixes(IspellDict *Conf)
> uses uninitialised ptr->issuffix:
>
> if (ptr == Conf->CompoundAffix ||
> ptr->issuffix != (ptr - 1)->issuffix ||

Yes, you are right. The second condition isn't right. Instead of
"ptr->issuffix != (ptr - 1)->issuffix" "Affix->type" should be checked
because we check for uniqueness of affixes.

The patch is attached.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> writes:
>> It looks that the following condition in NISortAffixes(IspellDict *Conf)
>> uses uninitialised ptr->issuffix:
>> if (ptr == Conf->CompoundAffix ||
>> ptr->issuffix != (ptr - 1)->issuffix ||

> Yes, you are right. The second condition isn't right. Instead of
> "ptr->issuffix != (ptr - 1)->issuffix" "Affix->type" should be checked
> because we check for uniqueness of affixes.

Yeah, existing code is clearly wrong, patch looks OK, will push.

But I see from the code coverage report that this bit is unexercised
in the regression tests. Any chance of getting a test that covers
this? I'm worried that this means we also lack any coverage of
cases where the CompoundAffix array has more than one entry.

regards, tom lane

On Thu, Apr 12, 2018 at 06:14:03PM -0400, Tom Lane wrote:
> Yeah, existing code is clearly wrong, patch looks OK, will push.

Thank you for the commit!

> But I see from the code coverage report that this bit is unexercised
> in the regression tests. Any chance of getting a test that covers
> this? I'm worried that this means we also lack any coverage of
> cases where the CompoundAffix array has more than one entry.

I attached the patch. It fixes the following:
- show an error if actual number of affix aliases is greater than
initial number. I wonder is it necessary. But I think it is better to
raise an error than crash, if you set wrong number for AF flag.
- improve code coverage for NISortAffixes().
- test regex_t expressions.
- test MergeAffix()
- test mkVoidAffix() better

The code coverage still isn't 100% for spell.c. But it is better than
earlier.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> writes:
> On Thu, Apr 12, 2018 at 06:14:03PM -0400, Tom Lane wrote:
>> But I see from the code coverage report that this bit is unexercised
>> in the regression tests. Any chance of getting a test that covers
>> this? I'm worried that this means we also lack any coverage of
>> cases where the CompoundAffix array has more than one entry.

> I attached the patch. It fixes the following:
> - show an error if actual number of affix aliases is greater than
> initial number. I wonder is it necessary. But I think it is better to
> raise an error than crash, if you set wrong number for AF flag.

Good idea, but I tweaked the message wording a bit.

> The code coverage still isn't 100% for spell.c. But it is better than
> earlier.

Indeed. Pushed, thanks!

regards, tom lane

On Fri, Apr 13, 2018 at 01:51:00PM -0400, Tom Lane wrote:
> > I attached the patch. It fixes the following:
> > - show an error if actual number of affix aliases is greater than
> > initial number. I wonder is it necessary. But I think it is better to
> > raise an error than crash, if you set wrong number for AF flag.
>
> Good idea, but I tweaked the message wording a bit.
>
> > The code coverage still isn't 100% for spell.c. But it is better than
> > earlier.
>
> Indeed. Pushed, thanks!

Thank you!

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company