Subject: Re: CREATE TEXT SEARCH DICTIONARY segfaulting on 9.6+
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>, pgsql-hackers@postgresql.org
Cc: Teodor Sigaev <teodor@sigaev.ru>
References: <20191013012610.2p2fp3zzpoav7jzf@development>
From: Arthur Zakirov <zaartur@gmail.com>
Message-ID: <3d98671e-f2ea-40ec-d0e7-d5c5cbb16ca3@gmail.com>
Date: Mon, 28 Oct 2019 11:59:01 +0900
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <20191013012610.2p2fp3zzpoav7jzf@development>
Content-Type: multipart/mixed;
 boundary="------------A058269387FF542C705CA8B1"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------A058269387FF542C705CA8B1
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello Tomas,

On 2019/10/13 10:26, Tomas Vondra wrote:
> over in pgsql-bugs [1] we got a report about CREATE TEXT SEARCH
> DICTIONARY causing segfaults on 12.0. Simply running
> 
>     CREATE TEXT SEARCH DICTIONARY hunspell_num (Template=ispell,
>     DictFile=hunspell_sample_num, AffFile=hunspell_sample_long);
> 
> does trigger a crash, 100% of the time. The crash was reported on 12.0,
> but it's in fact present since 9.6.
> 
> On 9.5 the example does not work, because that version does not (a)
> include the hunspell dictionaries used in the example, and (b) it does
> not support long flags. So even after copying the dictionaries and
> tweaking them a bit it still passes without a crash.

This crash is not because of long flags, but because of aliases (more 
thoughts below).

> Looking at the commit history of spell.c, there seems to be a bunch of
> commits in 2016 (e.g. f4ceed6ceba3) touching exactly this part of the
> code (hunspell), and it also correlates quite nicely with the affected
> branches (9.6+). So my best guess is it's a bug in those changes.

Yeah, there was a lot changes.

> So it simply grabs whatever it finds in the dict file, parses it and
> then (later) we use it as index to access the AffixData array, even if
> the value is way out of bounds.

Yes, we enter this code if an affix file defines aliases (AF parameter). 
AffixData array is used to store those aliases.

More about hunspell format you can find here:
https://linux.die.net/man/4/hunspell

In the example we have the following aliases:
AF 11
AF cZ		#1
AF cL		#2
...
AF sB		#11

And in the dictionary file we should use their indexes (from 1 to 11). 
These aliases defines set of flags and in the dict file we can use only 
single index:
book/3
book/11

but not:
book/3,4
book/2,11

I added checking of this last case in the attached patch. PostgreSQL 
will raise an error if it sees non-numeric and non-whitespace character 
after the index.

Aliases can be used with all flag types: 'default' (i.e. FM_CHAR), 
'long', and if I'm not mistaken 'num'.

> So I think we need some sort of cross-check here. We certainly need to
> make NISortDictionary() check the affix value is within AffixData
> bounds, and error out when the index is non-sensical (maybe negative
> and/or exceeding nAffixData).

I agree, I attached the patch which do this. I also added couple 
asserts, tests and fixed condition in getAffixFlagSet():

-		if (curaffix > 0 && curaffix <= Conf->nAffixData)
+		if (curaffix > 0 && curaffix < Conf->nAffixData)

I think it could be a bug, because curaffix can't be equal to 
Conf->nAffixData.

> Maybe there's a simple way to check if the affix/dict files match.

I'm not sure how to properly fix this either. The only thing we could 
check is commas in affix flags in a dict file:

book/302,301,202,303

FM_CHAR and FM_LONG dictionaries can't have commas. They should have the 
following affix flags:

book/sGsJpUsS	# 4 affixes for FM_LONG
book/GJUS	# 4 affixes for FM_CHAR

But I guess they could have numbers in flags (as help says "Set flag 
type. Default type is the extended ASCII (8-bit) character.") and other 
non alphanumeric characters (as some language dictionaries have):

book/s1s2s3s4	# 4 affixes for FM_LONG

-- 
Artur

--------------A058269387FF542C705CA8B1
Content-Type: text/plain; charset=UTF-8;
 name="fix_alias_index_handling.diff"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="fix_alias_index_handling.diff"

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3RzZWFyY2gvc3BlbGwuYyBiL3NyYy9iYWNrZW5k
L3RzZWFyY2gvc3BlbGwuYwppbmRleCBjNzE1ZjA2YjhlLi4xYjg3NjY2NTljIDEwMDY0NAot
LS0gYS9zcmMvYmFja2VuZC90c2VhcmNoL3NwZWxsLmMKKysrIGIvc3JjL2JhY2tlbmQvdHNl
YXJjaC9zcGVsbC5jCkBAIC00NTgsNiArNDU4LDggQEAgSXNBZmZpeEZsYWdJblVzZShJc3Bl
bGxEaWN0ICpDb25mLCBpbnQgYWZmaXgsIGNvbnN0IGNoYXIgKmFmZml4ZmxhZykKIAlpZiAo
KmFmZml4ZmxhZyA9PSAwKQogCQlyZXR1cm4gdHJ1ZTsKIAorCUFzc2VydChhZmZpeCA8IENv
bmYtPm5BZmZpeERhdGEpOworCiAJZmxhZ2N1ciA9IENvbmYtPkFmZml4RGF0YVthZmZpeF07
CiAKIAl3aGlsZSAoKmZsYWdjdXIpCkBAIC0xMTYwLDEzICsxMTYyLDE3IEBAIGdldEFmZml4
RmxhZ1NldChJc3BlbGxEaWN0ICpDb25mLCBjaGFyICpzKQogCQkJCQkoZXJyY29kZShFUlJD
T0RFX0NPTkZJR19GSUxFX0VSUk9SKSwKIAkJCQkJIGVycm1zZygiaW52YWxpZCBhZmZpeCBh
bGlhcyBcIiVzXCIiLCBzKSkpOwogCi0JCWlmIChjdXJhZmZpeCA+IDAgJiYgY3VyYWZmaXgg
PD0gQ29uZi0+bkFmZml4RGF0YSkKKwkJaWYgKGN1cmFmZml4ID4gMCAmJiBjdXJhZmZpeCA8
IENvbmYtPm5BZmZpeERhdGEpCiAKIAkJCS8qCiAJCQkgKiBEbyBub3Qgc3VidHJhY3QgMSBm
cm9tIGN1cmFmZml4IGJlY2F1c2UgZW1wdHkgc3RyaW5nIHdhcyBhZGRlZAogCQkJICogaW4g
TklJbXBvcnRPT0FmZml4ZXMKIAkJCSAqLwogCQkJcmV0dXJuIENvbmYtPkFmZml4RGF0YVtj
dXJhZmZpeF07CisJCWVsc2UgaWYgKGN1cmFmZml4ID4gQ29uZi0+bkFmZml4RGF0YSkKKwkJ
CWVyZXBvcnQoRVJST1IsCisJCQkJCShlcnJjb2RlKEVSUkNPREVfQ09ORklHX0ZJTEVfRVJS
T1IpLAorCQkJCQkgZXJybXNnKCJpbnZhbGlkIGFmZml4IGFsaWFzIFwiJXNcIiIsIHMpKSk7
CiAJCWVsc2UKIAkJCXJldHVybiBWb2lkU3RyaW5nOwogCX0KQEAgLTE1NjEsNiArMTU2Nyw4
IEBAIE1lcmdlQWZmaXgoSXNwZWxsRGljdCAqQ29uZiwgaW50IGExLCBpbnQgYTIpCiB7CiAJ
Y2hhcgkgICoqcHRyOwogCisJQXNzZXJ0KGExIDwgQ29uZi0+bkFmZml4RGF0YSAmJiBhMiA8
IENvbmYtPm5BZmZpeERhdGEpOworCiAJLyogRG8gbm90IG1lcmdlIGFmZml4IGZsYWdzIGlm
IG9uZSBvZiBhZmZpeCBmbGFncyBpcyBlbXB0eSAqLwogCWlmICgqQ29uZi0+QWZmaXhEYXRh
W2ExXSA9PSAnXDAnKQogCQlyZXR1cm4gYTI7CkBAIC0xNjAzLDkgKzE2MTEsMTAgQEAgTWVy
Z2VBZmZpeChJc3BlbGxEaWN0ICpDb25mLCBpbnQgYTEsIGludCBhMikKIHN0YXRpYyB1aW50
MzIKIG1ha2VDb21wb3VuZEZsYWdzKElzcGVsbERpY3QgKkNvbmYsIGludCBhZmZpeCkKIHsK
LQljaGFyCSAgICpzdHIgPSBDb25mLT5BZmZpeERhdGFbYWZmaXhdOworCUFzc2VydChhZmZp
eCA8IENvbmYtPm5BZmZpeERhdGEpOwogCi0JcmV0dXJuIChnZXRDb21wb3VuZEFmZml4Rmxh
Z1ZhbHVlKENvbmYsIHN0cikgJiBGRl9DT01QT1VOREZMQUdNQVNLKTsKKwlyZXR1cm4gKGdl
dENvbXBvdW5kQWZmaXhGbGFnVmFsdWUoQ29uZiwgQ29uZi0+QWZmaXhEYXRhW2FmZml4XSkg
JgorCQkJRkZfQ09NUE9VTkRGTEFHTUFTSyk7CiB9CiAKIC8qCkBAIC0xNzI1LDYgKzE3MzQs
MTYgQEAgTklTb3J0RGljdGlvbmFyeShJc3BlbGxEaWN0ICpDb25mKQogCQkJCQkJCShlcnJj
b2RlKEVSUkNPREVfQ09ORklHX0ZJTEVfRVJST1IpLAogCQkJCQkJCSBlcnJtc2coImludmFs
aWQgYWZmaXggYWxpYXMgXCIlc1wiIiwKIAkJCQkJCQkJCUNvbmYtPlNwZWxsW2ldLT5wLmZs
YWcpKSk7CisJCQkJaWYgKGN1cmFmZml4IDwgMCB8fCBjdXJhZmZpeCA+PSBDb25mLT5uQWZm
aXhEYXRhKQorCQkJCQllcmVwb3J0KEVSUk9SLAorCQkJCQkJCShlcnJjb2RlKEVSUkNPREVf
Q09ORklHX0ZJTEVfRVJST1IpLAorCQkJCQkJCSBlcnJtc2coImludmFsaWQgYWZmaXggYWxp
YXMgXCIlc1wiIiwKKwkJCQkJCQkJCUNvbmYtPlNwZWxsW2ldLT5wLmZsYWcpKSk7CisJCQkJ
aWYgKCplbmQgIT0gJ1wwJyAmJiAhdF9pc2RpZ2l0KGVuZCkgJiYgIXRfaXNzcGFjZShlbmQp
KQorCQkJCQllcmVwb3J0KEVSUk9SLAorCQkJCQkJCShlcnJjb2RlKEVSUkNPREVfQ09ORklH
X0ZJTEVfRVJST1IpLAorCQkJCQkJCSBlcnJtc2coImludmFsaWQgYWZmaXggYWxpYXMgXCIl
c1wiIiwKKwkJCQkJCQkJCUNvbmYtPlNwZWxsW2ldLT5wLmZsYWcpKSk7CiAJCQl9CiAJCQll
bHNlCiAJCQl7CmRpZmYgLS1naXQgYS9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL3RzZGlj
dHMub3V0IGIvc3JjL3Rlc3QvcmVncmVzcy9leHBlY3RlZC90c2RpY3RzLm91dAppbmRleCAy
NTI0ZWMyNzY4Li41YTkyN2JlOTQ4IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9yZWdyZXNzL2V4
cGVjdGVkL3RzZGljdHMub3V0CisrKyBiL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvdHNk
aWN0cy5vdXQKQEAgLTQxMyw2ICs0MTMsNDAgQEAgU0VMRUNUIHRzX2xleGl6ZSgnaHVuc3Bl
bGxfbnVtJywgJ2Zvb3RiYWxseWtsdWJiZXInKTsKICB7Zm9vdCxiYWxsLGtsdWJiZXJ9CiAo
MSByb3cpCiAKKy0tIFRlc3Qgc3VpdGFiaWxpdHkgb2YgYWZmaXggYW5kIGRpY3QgZmlsZXMK
K0NSRUFURSBURVhUIFNFQVJDSCBESUNUSU9OQVJZIGh1bnNwZWxsX2VyciAoCisJCQkJCQlU
ZW1wbGF0ZT1pc3BlbGwsCisJCQkJCQlEaWN0RmlsZT1pc3BlbGxfc2FtcGxlLAorCQkJCQkJ
QWZmRmlsZT1odW5zcGVsbF9zYW1wbGVfbG9uZworKTsKK0VSUk9SOiAgaW52YWxpZCBhZmZp
eCBhbGlhcyAiR0pVUyIKK0NSRUFURSBURVhUIFNFQVJDSCBESUNUSU9OQVJZIGh1bnNwZWxs
X2VyciAoCisJCQkJCQlUZW1wbGF0ZT1pc3BlbGwsCisJCQkJCQlEaWN0RmlsZT1pc3BlbGxf
c2FtcGxlLAorCQkJCQkJQWZmRmlsZT1odW5zcGVsbF9zYW1wbGVfbnVtCispOworRVJST1I6
ICBpbnZhbGlkIGFmZml4IGZsYWcgIlNaXCIKK0NSRUFURSBURVhUIFNFQVJDSCBESUNUSU9O
QVJZIGh1bnNwZWxsX2ludmFsaWRfMSAoCisJCQkJCQlUZW1wbGF0ZT1pc3BlbGwsCisJCQkJ
CQlEaWN0RmlsZT1odW5zcGVsbF9zYW1wbGVfbG9uZywKKwkJCQkJCUFmZkZpbGU9aXNwZWxs
X3NhbXBsZQorKTsKK0NSRUFURSBURVhUIFNFQVJDSCBESUNUSU9OQVJZIGh1bnNwZWxsX2lu
dmFsaWRfMiAoCisJCQkJCQlUZW1wbGF0ZT1pc3BlbGwsCisJCQkJCQlEaWN0RmlsZT1odW5z
cGVsbF9zYW1wbGVfbG9uZywKKwkJCQkJCUFmZkZpbGU9aHVuc3BlbGxfc2FtcGxlX251bQor
KTsKK0NSRUFURSBURVhUIFNFQVJDSCBESUNUSU9OQVJZIGh1bnNwZWxsX2ludmFsaWRfMyAo
CisJCQkJCQlUZW1wbGF0ZT1pc3BlbGwsCisJCQkJCQlEaWN0RmlsZT1odW5zcGVsbF9zYW1w
bGVfbnVtLAorCQkJCQkJQWZmRmlsZT1pc3BlbGxfc2FtcGxlCispOworQ1JFQVRFIFRFWFQg
U0VBUkNIIERJQ1RJT05BUlkgaHVuc3BlbGxfZXJyICgKKwkJCQkJCVRlbXBsYXRlPWlzcGVs
bCwKKwkJCQkJCURpY3RGaWxlPWh1bnNwZWxsX3NhbXBsZV9udW0sCisJCQkJCQlBZmZGaWxl
PWh1bnNwZWxsX3NhbXBsZV9sb25nCispOworRVJST1I6ICBpbnZhbGlkIGFmZml4IGFsaWFz
ICIzMDIsMzAxLDIwMiwzMDMiCiAtLSBTeW5vbnltIGRpY3Rpb25hcnkKIENSRUFURSBURVhU
IFNFQVJDSCBESUNUSU9OQVJZIHN5bm9ueW0gKAogCQkJCQkJVGVtcGxhdGU9c3lub255bSwK
ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlZ3Jlc3Mvc3FsL3RzZGljdHMuc3FsIGIvc3JjL3Rl
c3QvcmVncmVzcy9zcWwvdHNkaWN0cy5zcWwKaW5kZXggNjA5MDZmNjU0OS4uOTA4ZTY3NTUw
MSAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvcmVncmVzcy9zcWwvdHNkaWN0cy5zcWwKKysrIGIv
c3JjL3Rlc3QvcmVncmVzcy9zcWwvdHNkaWN0cy5zcWwKQEAgLTEwMSw2ICsxMDEsNDMgQEAg
U0VMRUNUIHRzX2xleGl6ZSgnaHVuc3BlbGxfbnVtJywgJ2Zvb3RiYWxsa2x1YmJlcicpOwog
U0VMRUNUIHRzX2xleGl6ZSgnaHVuc3BlbGxfbnVtJywgJ2JhbGx5a2x1YmJlcicpOwogU0VM
RUNUIHRzX2xleGl6ZSgnaHVuc3BlbGxfbnVtJywgJ2Zvb3RiYWxseWtsdWJiZXInKTsKIAor
LS0gVGVzdCBzdWl0YWJpbGl0eSBvZiBhZmZpeCBhbmQgZGljdCBmaWxlcworQ1JFQVRFIFRF
WFQgU0VBUkNIIERJQ1RJT05BUlkgaHVuc3BlbGxfZXJyICgKKwkJCQkJCVRlbXBsYXRlPWlz
cGVsbCwKKwkJCQkJCURpY3RGaWxlPWlzcGVsbF9zYW1wbGUsCisJCQkJCQlBZmZGaWxlPWh1
bnNwZWxsX3NhbXBsZV9sb25nCispOworCitDUkVBVEUgVEVYVCBTRUFSQ0ggRElDVElPTkFS
WSBodW5zcGVsbF9lcnIgKAorCQkJCQkJVGVtcGxhdGU9aXNwZWxsLAorCQkJCQkJRGljdEZp
bGU9aXNwZWxsX3NhbXBsZSwKKwkJCQkJCUFmZkZpbGU9aHVuc3BlbGxfc2FtcGxlX251bQor
KTsKKworQ1JFQVRFIFRFWFQgU0VBUkNIIERJQ1RJT05BUlkgaHVuc3BlbGxfaW52YWxpZF8x
ICgKKwkJCQkJCVRlbXBsYXRlPWlzcGVsbCwKKwkJCQkJCURpY3RGaWxlPWh1bnNwZWxsX3Nh
bXBsZV9sb25nLAorCQkJCQkJQWZmRmlsZT1pc3BlbGxfc2FtcGxlCispOworCitDUkVBVEUg
VEVYVCBTRUFSQ0ggRElDVElPTkFSWSBodW5zcGVsbF9pbnZhbGlkXzIgKAorCQkJCQkJVGVt
cGxhdGU9aXNwZWxsLAorCQkJCQkJRGljdEZpbGU9aHVuc3BlbGxfc2FtcGxlX2xvbmcsCisJ
CQkJCQlBZmZGaWxlPWh1bnNwZWxsX3NhbXBsZV9udW0KKyk7CisKK0NSRUFURSBURVhUIFNF
QVJDSCBESUNUSU9OQVJZIGh1bnNwZWxsX2ludmFsaWRfMyAoCisJCQkJCQlUZW1wbGF0ZT1p
c3BlbGwsCisJCQkJCQlEaWN0RmlsZT1odW5zcGVsbF9zYW1wbGVfbnVtLAorCQkJCQkJQWZm
RmlsZT1pc3BlbGxfc2FtcGxlCispOworCitDUkVBVEUgVEVYVCBTRUFSQ0ggRElDVElPTkFS
WSBodW5zcGVsbF9lcnIgKAorCQkJCQkJVGVtcGxhdGU9aXNwZWxsLAorCQkJCQkJRGljdEZp
bGU9aHVuc3BlbGxfc2FtcGxlX251bSwKKwkJCQkJCUFmZkZpbGU9aHVuc3BlbGxfc2FtcGxl
X2xvbmcKKyk7CisKIC0tIFN5bm9ueW0gZGljdGlvbmFyeQogQ1JFQVRFIFRFWFQgU0VBUkNI
IERJQ1RJT05BUlkgc3lub255bSAoCiAJCQkJCQlUZW1wbGF0ZT1zeW5vbnltLAo=
--------------A058269387FF542C705CA8B1--