Thread: BUG #15150: Reading uninitialised value in NISortAffixes(tsearch/spell.c)
BUG #15150: Reading uninitialised value in NISortAffixes(tsearch/spell.c)
From
PG Bug reporting form
Date:
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 ||
Re: BUG #15150: Reading uninitialised value in NISortAffixes(tsearch/spell.c)
From
Arthur Zakirov
Date:
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
Attachment
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
Re: BUG #15150: Reading uninitialised value in NISortAffixes(tsearch/spell.c)
From
Arthur Zakirov
Date:
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
Attachment
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
Re: BUG #15150: Reading uninitialised value in NISortAffixes(tsearch/spell.c)
From
Arthur Zakirov
Date:
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