Horton

Thread

Commits

  1. Improve regression test coverage for src/backend/tsearch/spell.c.

  2. Fix bogus affix-merging code.

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

    The Post Office <noreply@postgresql.org> — 2018-04-12T11:41:17Z

    The following bug has been logged on the website:

Bug reference:      15150
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.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 ||

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

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-04-12T11:56:39Z

    Hello,

> Bug reference:      15150
> Logged by:          Alexander Lakhin
> Email address:      exclusion@gmail.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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-12T22:14:03Z

    Arthur Zakirov <a.zakirov@postgrespro.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

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

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-04-13T11:34:48Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-13T17:51:00Z

    Arthur Zakirov <a.zakirov@postgrespro.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

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

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2018-04-13T19:05:30Z

    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