pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
6+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2018-04-13 8bf358c18e Improve regression test coverage for src/backend/tsearch/spell.c.
2018-04-12 65a69dfa08 f71d803c8d 40132187ed 906e44d4dc 0f439c8dd2 ac8ea0f27f Fix bogus affix-merging code.
* BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
@ 2018-04-12 11:41 PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
0 siblings, 1 reply; 6+ messages in thread
From: PG Bug reporting form @ 2018-04-12 11:41 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
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 ||
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
@ 2018-04-12 11:56 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-12 22:14 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Arthur Zakirov @ 2018-04-12 11:56 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
fix-NISortAffixes-condition.patch (text/plain, 909B) download
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index b9fdd77e19..65d99da292 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -1963,8 +1963,10 @@ NISortAffixes(IspellDict *Conf)
if ((Affix->flagflags & FF_COMPOUNDFLAG) && Affix->replen > 0 &&
isAffixInUse(Conf, Affix->flag))
{
+ bool issuffix = (Affix->type == FF_SUFFIX);
+
if (ptr == Conf->CompoundAffix ||
- ptr->issuffix != (ptr - 1)->issuffix ||
+ issuffix != (ptr - 1)->issuffix ||
strbncmp((const unsigned char *) (ptr - 1)->affix,
(const unsigned char *) Affix->repl,
(ptr - 1)->len))
@@ -1972,7 +1974,7 @@ NISortAffixes(IspellDict *Conf)
/* leave only unique and minimals suffixes */
ptr->affix = Affix->repl;
ptr->len = Affix->replen;
- ptr->issuffix = (Affix->type == FF_SUFFIX);
+ ptr->issuffix = issuffix;
ptr++;
}
}
* Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
@ 2018-04-12 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 11:34 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2018-04-12 22:14 UTC (permalink / raw)
To: Arthur Zakirov <a.zakirov@postgrespro.ru>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-12 22:14 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-04-13 11:34 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-13 17:51 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Arthur Zakirov @ 2018-04-13 11:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
improve-spell-codecoverage.patch (text/plain, 5.9K) download
diff --git a/src/backend/tsearch/dicts/hunspell_sample_long.affix b/src/backend/tsearch/dicts/hunspell_sample_long.affix
index fc0d485a4d..d5df7a31f3 100644
--- a/src/backend/tsearch/dicts/hunspell_sample_long.affix
+++ b/src/backend/tsearch/dicts/hunspell_sample_long.affix
@@ -1,15 +1,23 @@
FLAG long
-AF 7
+AF 11
AF cZ #1
AF cL #2
AF sGsJpUsS #3
AF sSpB #4
AF cZsS #5
-AF sScZs\ #6
+AF sScZs\sE #6
AF sA #7
+AF CaCp #8
+AF CcCp #9
+AF sD #10
+AF sB #11
COMPOUNDFLAG cZ
+COMPOUNDBEGIN Ca
+COMPOUNDMIDDLE Cb
+COMPOUNDEND Cc
+COMPOUNDPERMITFLAG Cp
ONLYINCOMPOUND cL
PFX pB Y 1
@@ -28,7 +36,18 @@ SFX sS Y 1
SFX sS 0 S [^SXZHY]
SFX sA Y 1
-SFX sA Y IES [^AEIOU]Y
+SFX sA Y IES [^AEIOU]Y{1}
+SFX sB Y 1
+SFX sB 0 ED K{1}
+
+# Affixes with compound flags
SFX s\ N 1
SFX s\ 0 Y/2 [^Y]
+
+SFX sE N 1
+SFX sE 0 S/2 [^S]
+
+# Check duplicate affixes
+SFX sD N 1
+SFX sD 0 S/2 [^S]
diff --git a/src/backend/tsearch/dicts/hunspell_sample_long.dict b/src/backend/tsearch/dicts/hunspell_sample_long.dict
index 96ecbf007a..5f60a07ce2 100644
--- a/src/backend/tsearch/dicts/hunspell_sample_long.dict
+++ b/src/backend/tsearch/dicts/hunspell_sample_long.dict
@@ -1,4 +1,5 @@
book/3
+book/11
booking/4
footballklubber
foot/5
@@ -6,3 +7,5 @@ football/1
ball/6
klubber/1
sky/7
+ex-/8
+machina/9
\ No newline at end of file
diff --git a/src/backend/tsearch/dicts/hunspell_sample_num.affix b/src/backend/tsearch/dicts/hunspell_sample_num.affix
index 5f2f8e9bca..0c4766a191 100644
--- a/src/backend/tsearch/dicts/hunspell_sample_num.affix
+++ b/src/backend/tsearch/dicts/hunspell_sample_num.affix
@@ -18,6 +18,14 @@ SFX 302 0 ING [^E]
SFX 303 Y 1
SFX 303 0 S [^SXZHY]
+# Remove ED suffix from lexeme for base words with K ending
+SFX 306 Y 1
+SFX 306 0 ED K{1}
+
+# Just add Y to lexeme for base words with Y ending
+SFX 307 Y 1
+SFX 307 Y 0 Y*
+
SFX 304 Y 1
SFX 304 Y IES [^AEIOU]Y
diff --git a/src/backend/tsearch/dicts/hunspell_sample_num.dict b/src/backend/tsearch/dicts/hunspell_sample_num.dict
index 9db29dc780..fbc321d5b1 100644
--- a/src/backend/tsearch/dicts/hunspell_sample_num.dict
+++ b/src/backend/tsearch/dicts/hunspell_sample_num.dict
@@ -1,8 +1,9 @@
book/302,301,202,303
+book/306
booking/303,201
footballklubber
foot/101,303
football/101
ball/303,101,305
klubber/101
-sky/304
+sky/304,307
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index 65d99da292..68049dda7c 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -1326,6 +1326,11 @@ NIImportOOAffixes(IspellDict *Conf, const char *filename)
Conf->AffixData[curaffix] = cpstrdup(Conf, sflag);
curaffix++;
}
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("actual number of aliases exceeds initial "
+ "number %d", naffix)));
}
goto nextline;
}
diff --git a/src/test/regress/expected/tsdicts.out b/src/test/regress/expected/tsdicts.out
index 0c1d7c7675..2524ec2768 100644
--- a/src/test/regress/expected/tsdicts.out
+++ b/src/test/regress/expected/tsdicts.out
@@ -263,6 +263,12 @@ SELECT ts_lexize('hunspell_long', 'unbook');
{book}
(1 row)
+SELECT ts_lexize('hunspell_long', 'booked');
+ ts_lexize
+-----------
+ {book}
+(1 row)
+
SELECT ts_lexize('hunspell_long', 'footklubber');
ts_lexize
----------------
@@ -281,12 +287,24 @@ SELECT ts_lexize('hunspell_long', 'ballyklubber');
{ball,klubber}
(1 row)
+SELECT ts_lexize('hunspell_long', 'ballsklubber');
+ ts_lexize
+----------------
+ {ball,klubber}
+(1 row)
+
SELECT ts_lexize('hunspell_long', 'footballyklubber');
ts_lexize
---------------------
{foot,ball,klubber}
(1 row)
+SELECT ts_lexize('hunspell_long', 'ex-machina');
+ ts_lexize
+---------------
+ {ex-,machina}
+(1 row)
+
-- Test ISpell dictionary with hunspell affix file with FLAG num parameter
CREATE TEXT SEARCH DICTIONARY hunspell_num (
Template=ispell,
@@ -299,6 +317,12 @@ SELECT ts_lexize('hunspell_num', 'skies');
{sky}
(1 row)
+SELECT ts_lexize('hunspell_num', 'sk');
+ ts_lexize
+-----------
+ {sky}
+(1 row)
+
SELECT ts_lexize('hunspell_num', 'bookings');
ts_lexize
----------------
@@ -359,6 +383,12 @@ SELECT ts_lexize('hunspell_num', 'unbook');
{book}
(1 row)
+SELECT ts_lexize('hunspell_num', 'booked');
+ ts_lexize
+-----------
+ {book}
+(1 row)
+
SELECT ts_lexize('hunspell_num', 'footklubber');
ts_lexize
----------------
diff --git a/src/test/regress/sql/tsdicts.sql b/src/test/regress/sql/tsdicts.sql
index 1633c0d066..60906f6549 100644
--- a/src/test/regress/sql/tsdicts.sql
+++ b/src/test/regress/sql/tsdicts.sql
@@ -66,11 +66,14 @@ SELECT ts_lexize('hunspell_long', 'rebook');
SELECT ts_lexize('hunspell_long', 'unbookings');
SELECT ts_lexize('hunspell_long', 'unbooking');
SELECT ts_lexize('hunspell_long', 'unbook');
+SELECT ts_lexize('hunspell_long', 'booked');
SELECT ts_lexize('hunspell_long', 'footklubber');
SELECT ts_lexize('hunspell_long', 'footballklubber');
SELECT ts_lexize('hunspell_long', 'ballyklubber');
+SELECT ts_lexize('hunspell_long', 'ballsklubber');
SELECT ts_lexize('hunspell_long', 'footballyklubber');
+SELECT ts_lexize('hunspell_long', 'ex-machina');
-- Test ISpell dictionary with hunspell affix file with FLAG num parameter
CREATE TEXT SEARCH DICTIONARY hunspell_num (
@@ -80,6 +83,7 @@ CREATE TEXT SEARCH DICTIONARY hunspell_num (
);
SELECT ts_lexize('hunspell_num', 'skies');
+SELECT ts_lexize('hunspell_num', 'sk');
SELECT ts_lexize('hunspell_num', 'bookings');
SELECT ts_lexize('hunspell_num', 'booking');
SELECT ts_lexize('hunspell_num', 'foot');
@@ -90,6 +94,7 @@ SELECT ts_lexize('hunspell_num', 'rebook');
SELECT ts_lexize('hunspell_num', 'unbookings');
SELECT ts_lexize('hunspell_num', 'unbooking');
SELECT ts_lexize('hunspell_num', 'unbook');
+SELECT ts_lexize('hunspell_num', 'booked');
SELECT ts_lexize('hunspell_num', 'footklubber');
SELECT ts_lexize('hunspell_num', 'footballklubber');
* Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-12 22:14 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 11:34 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
@ 2018-04-13 17:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 19:05 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2018-04-13 17:51 UTC (permalink / raw)
To: Arthur Zakirov <a.zakirov@postgrespro.ru>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c)
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-12 22:14 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 11:34 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-13 17:51 ` Re: BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-04-13 19:05 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
0 siblings, 0 replies; 6+ messages in thread
From: Arthur Zakirov @ 2018-04-13 19:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2018-04-13 19:05 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-04-12 11:41 BUG #15150: Reading uninitialised value in NISortAffixes (tsearch/spell.c) PG Bug reporting form <noreply@postgresql.org>
2018-04-12 11:56 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-12 22:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 11:34 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
2018-04-13 17:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-04-13 19:05 ` Arthur Zakirov <a.zakirov@postgrespro.ru>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox