Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rMXVs-0056gl-UP for pgsql-bugs@arkaria.postgresql.org; Sun, 07 Jan 2024 18:04:46 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rMXVr-00H2is-CF for pgsql-bugs@arkaria.postgresql.org; Sun, 07 Jan 2024 18:04:43 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rMXVq-00H2ii-TT for pgsql-bugs@lists.postgresql.org; Sun, 07 Jan 2024 18:04:43 +0000 Received: from forwardcorp1c.mail.yandex.net ([2a02:6b8:c03:500:1:45:d181:df01]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rMXVo-000LWt-5Q for pgsql-bugs@postgresql.org; Sun, 07 Jan 2024 18:04:42 +0000 Received: from mail-nwsmtp-smtp-corp-main-26.myt.yp-c.yandex.net (mail-nwsmtp-smtp-corp-main-26.myt.yp-c.yandex.net [IPv6:2a02:6b8:c00:1918:0:640:433:0]) by forwardcorp1c.mail.yandex.net (Yandex) with ESMTPS id 865CB6135E; Sun, 7 Jan 2024 21:04:37 +0300 (MSK) Received: from smtpclient.apple (unknown [2a02:6b8:b081:6516::1:38]) by mail-nwsmtp-smtp-corp-main-26.myt.yp-c.yandex.net (smtpcorp/Yandex) with ESMTPSA id Z4kwHDJIeKo0-zZhsX9ZC; Sun, 07 Jan 2024 21:04:37 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex-team.ru; s=default; t=1704650677; bh=lTqQkyyaI9qK/Zcn6FuNVA0W5vVHAT401fkwpteKHDk=; h=References:To:Cc:In-Reply-To:Date:From:Message-Id:Subject; b=P9C8rRQysLyo19JK9RTwINYsjIEGA5sGV7UKqB7THQlghGlRefpOJ2y1tXmbT/Zkq /oLPW9Z+AeqJHkhM4q8EPYXwnuU5m9y19ttAHc9gmB15U+rqtQ++cJp4G2lvQJB9qP Tt6G43ed2NgemFHE/ZC1on9g2N+XBCCtV2TUSlf4= Authentication-Results: mail-nwsmtp-smtp-corp-main-26.myt.yp-c.yandex.net; dkim=pass header.i=@yandex-team.ru From: "Andrey M. Borodin" Message-Id: <0FDE2089-D306-4CBB-AD1F-EC4B419E3B33@yandex-team.ru> Content-Type: multipart/mixed; boundary="Apple-Mail=_1355B393-CA22-494E-A3D2-53080FC2FFDD" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.4\)) Subject: Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum Date: Sun, 7 Jan 2024 23:04:35 +0500 In-Reply-To: <7bdbe559-d61a-4ae4-a6e1-48abdf3024cc@postgrespro.ru> Cc: pgsql-bugs@postgresql.org, y sokolov , Alexander Lakhin To: Michael Zhilin References: <7bdbe559-d61a-4ae4-a6e1-48abdf3024cc@postgrespro.ru> X-Mailer: Apple Mail (2.3696.120.41.1.4) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_1355B393-CA22-494E-A3D2-53080FC2FFDD Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On 14 Dec 2023, at 21:18, Michael Zhilin = wrote: I've checked that: * bug is reproduced by the test in the patch * bug is fixed by the patch * fix seems idiomatic, similar to nearby code Patch needed a rebase, so please find attached rebased version. I did = not change anything. I see that using a temp file in PG_ABS_SRCDIR is common approach. But = still I want to ask, maybe can we develop some clever way to reproduce = the bug without external file? Also, maybe nearby code would be slightly more readable, if = normalized[i] was a local variable. And one last question about the line: char *data =3D palloc(len); what if data is somehow corrupted here... are there enough sanity checks = that we won't palloc(-1) or something like that? Won't we memcpy() from some other memory when len is bogus? Besides this paranoid questions, I think that this patch is ready for = committer. Thanks! Best regards, Andrey Borodin. --Apple-Mail=_1355B393-CA22-494E-A3D2-53080FC2FFDD Content-Disposition: attachment; filename=v1rebased-0001-contrib-amcheck-must-support-different-hea.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="v1rebased-0001-contrib-amcheck-must-support-different-hea.patch" Content-Transfer-Encoding: quoted-printable =46rom=20f05a9ad52aa6552c9cae5dcc712a168e08c67d80=20Mon=20Sep=2017=20= 00:00:00=202001=0AFrom:=20Michael=20Zhilin=20=0A= Date:=20Thu,=2014=20Dec=202023=2016:08:15=20+0300=0ASubject:=20[PATCH=20= v1rebased]=20contrib/amcheck:=20must=20support=20different=20header=20= size=0A=20of=20short=20varlena=20datum=0A=0A---=0A=20= contrib/amcheck/expected/check_btree.out=20|=2019=20+++++++++++++=0A=20= contrib/amcheck/sql/check_btree.sql=20=20=20=20=20=20|=2017=20= +++++++++++=0A=20contrib/amcheck/verify_nbtree.c=20=20=20=20=20=20=20=20=20= =20|=2036=20++++++++++++++++++++----=0A=203=20files=20changed,=2067=20= insertions(+),=205=20deletions(-)=0A=0Adiff=20--git=20= a/contrib/amcheck/expected/check_btree.out=20= b/contrib/amcheck/expected/check_btree.out=0Aindex=20= 86b38d93f4..1ab1bd260c=20100644=0A---=20= a/contrib/amcheck/expected/check_btree.out=0A+++=20= b/contrib/amcheck/expected/check_btree.out=0A@@=20-1,3=20+1,5=20@@=0A+--=20= directory=20paths=20are=20passed=20to=20us=20in=20environment=20= variables=0A+\getenv=20abs_srcdir=20PG_ABS_SRCDIR=0A=20CREATE=20TABLE=20= bttest_a(id=20int8);=0A=20CREATE=20TABLE=20bttest_b(id=20int8);=0A=20= CREATE=20TABLE=20bttest_multi(id=20int8,=20data=20int8);=0A@@=20-240,6=20= +242,22=20@@=20SELECT=20bt_index_check('bttest_unique_nulls_b_c_idx',=20= heapallindexed=20=3D>=20true,=20che=0A=20=20=0A=20(1=20row)=0A=20=0A+--=0A= +--=20BUG:=20must=20support=20different=20header=20size=20of=20short=20= varlena=20datum=0A+--=0A+CREATE=20TABLE=20varlena_bug=20(v=20text);=0A= +ALTER=20TABLE=20varlena_bug=20ALTER=20column=20v=20SET=20storage=20= plain;=0A+INSERT=20INTO=20varlena_bug=20VALUES=20('x');=0A+\set=20= filename=20:abs_srcdir=20'/results/varlena_bug.dmp'=0A+COPY=20= varlena_bug=20TO=20:'filename';=0A+COPY=20varlena_bug=20FROM=20= :'filename';=0A+CREATE=20INDEX=20varlena_bug_idx=20on=20varlena_bug(v);=0A= +SELECT=20bt_index_check('varlena_bug_idx',=20true);=0A+=20= bt_index_check=20=0A+----------------=0A+=20=0A+(1=20row)=0A+=0A=20--=20= cleanup=0A=20DROP=20TABLE=20bttest_a;=0A=20DROP=20TABLE=20bttest_b;=0A@@=20= -250,3=20+268,4=20@@=20DROP=20FUNCTION=20ifun(int8);=0A=20DROP=20TABLE=20= bttest_unique_nulls;=0A=20DROP=20OWNED=20BY=20regress_bttest_role;=20--=20= permissions=0A=20DROP=20ROLE=20regress_bttest_role;=0A+DROP=20TABLE=20= varlena_bug;=0Adiff=20--git=20a/contrib/amcheck/sql/check_btree.sql=20= b/contrib/amcheck/sql/check_btree.sql=0Aindex=20aa461f7fb9..4daed676c2=20= 100644=0A---=20a/contrib/amcheck/sql/check_btree.sql=0A+++=20= b/contrib/amcheck/sql/check_btree.sql=0A@@=20-1,3=20+1,6=20@@=0A+--=20= directory=20paths=20are=20passed=20to=20us=20in=20environment=20= variables=0A+\getenv=20abs_srcdir=20PG_ABS_SRCDIR=0A+=0A=20CREATE=20= TABLE=20bttest_a(id=20int8);=0A=20CREATE=20TABLE=20bttest_b(id=20int8);=0A= =20CREATE=20TABLE=20bttest_multi(id=20int8,=20data=20int8);=0A@@=20= -148,6=20+151,19=20@@=20SELECT=20= bt_index_check('bttest_unique_nulls_c_key',=20heapallindexed=20=3D>=20= true,=20check=0A=20CREATE=20INDEX=20on=20bttest_unique_nulls=20(b,c);=0A=20= SELECT=20bt_index_check('bttest_unique_nulls_b_c_idx',=20heapallindexed=20= =3D>=20true,=20checkunique=20=3D>=20true);=0A=20=0A+--=0A+--=20BUG:=20= must=20support=20different=20header=20size=20of=20short=20varlena=20= datum=0A+--=0A+=0A+CREATE=20TABLE=20varlena_bug=20(v=20text);=0A+ALTER=20= TABLE=20varlena_bug=20ALTER=20column=20v=20SET=20storage=20plain;=0A= +INSERT=20INTO=20varlena_bug=20VALUES=20('x');=0A+\set=20filename=20= :abs_srcdir=20'/results/varlena_bug.dmp'=0A+COPY=20varlena_bug=20TO=20= :'filename';=0A+COPY=20varlena_bug=20FROM=20:'filename';=0A+CREATE=20= INDEX=20varlena_bug_idx=20on=20varlena_bug(v);=0A+SELECT=20= bt_index_check('varlena_bug_idx',=20true);=0A+=0A=20--=20cleanup=0A=20= DROP=20TABLE=20bttest_a;=0A=20DROP=20TABLE=20bttest_b;=0A@@=20-158,3=20= +174,4=20@@=20DROP=20FUNCTION=20ifun(int8);=0A=20DROP=20TABLE=20= bttest_unique_nulls;=0A=20DROP=20OWNED=20BY=20regress_bttest_role;=20--=20= permissions=0A=20DROP=20ROLE=20regress_bttest_role;=0A+DROP=20TABLE=20= varlena_bug;=0Adiff=20--git=20a/contrib/amcheck/verify_nbtree.c=20= b/contrib/amcheck/verify_nbtree.c=0Aindex=2091caa53dd8..e7f01c2add=20= 100644=0A---=20a/contrib/amcheck/verify_nbtree.c=0A+++=20= b/contrib/amcheck/verify_nbtree.c=0A@@=20-2942,7=20+2942,7=20@@=20= bt_normalize_tuple(BtreeCheckState=20*state,=20IndexTuple=20itup)=0A=20=09= TupleDesc=09tupleDescriptor=20=3D=20RelationGetDescr(state->rel);=0A=20=09= Datum=09=09normalized[INDEX_MAX_KEYS];=0A=20=09bool=09=09= isnull[INDEX_MAX_KEYS];=0A-=09bool=09=09toast_free[INDEX_MAX_KEYS];=0A+=09= bool=09=09need_free[INDEX_MAX_KEYS];=0A=20=09bool=09=09formnewtup=20=3D=20= false;=0A=20=09IndexTuple=09reformed;=0A=20=09int=09=09=09i;=0A@@=20= -2961,7=20+2961,7=20@@=20bt_normalize_tuple(BtreeCheckState=20*state,=20= IndexTuple=20itup)=0A=20=09=09att=20=3D=20TupleDescAttr(tupleDescriptor,=20= i);=0A=20=0A=20=09=09/*=20Assume=20untoasted/already=20normalized=20= datum=20initially=20*/=0A-=09=09toast_free[i]=20=3D=20false;=0A+=09=09= need_free[i]=20=3D=20false;=0A=20=09=09normalized[i]=20=3D=20= index_getattr(itup,=20att->attnum,=0A=20=09=09=09=09=09=09=09=09=09=20=20= tupleDescriptor,=0A=20=09=09=09=09=09=09=09=09=09=20=20&isnull[i]);=0A@@=20= -2973,6=20+2973,7=20@@=20bt_normalize_tuple(BtreeCheckState=20*state,=20= IndexTuple=20itup)=0A=20=09=09=20*=20index=20without=20further=20= processing,=20so=20an=20external=20varlena=20header=0A=20=09=09=20*=20= should=20never=20be=20encountered=20here=0A=20=09=09=20*/=0A+=0A=20=09=09= if=20(VARATT_IS_EXTERNAL(DatumGetPointer(normalized[i])))=0A=20=09=09=09= ereport(ERROR,=0A=20=09=09=09=09=09(errcode(ERRCODE_INDEX_CORRUPTED),=0A= @@=20-2984,11=20+2985,32=20@@=20bt_normalize_tuple(BtreeCheckState=20= *state,=20IndexTuple=20itup)=0A=20=09=09{=0A=20=09=09=09formnewtup=20=3D=20= true;=0A=20=09=09=09normalized[i]=20=3D=20= PointerGetDatum(PG_DETOAST_DATUM(normalized[i]));=0A-=09=09=09= toast_free[i]=20=3D=20true;=0A+=09=09=09need_free[i]=20=3D=20true;=0A=20=09= =09}=0A+=09=09/*=0A+=09=09=20*=20Short=20tuples=20may=20have=201B=20or=20= 4B=20header.=20Convert=204B=20header=20of=20short=0A+=09=09=20*=20tuples=20= to=201B=0A+=09=09=20*/=0A+=09=09else=20if=20= (VARATT_CAN_MAKE_SHORT(DatumGetPointer(normalized[i])))=0A+=09=09{=0A+=09= =09=09/*=20convert=20to=20short=20varlena=20*/=0A+=09=09=09Size=20len=20= =3D=20VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer(normalized[i]));=0A+=09= =09=09char=20*data=20=3D=20palloc(len);=0A+=0A+=09=09=09= SET_VARSIZE_SHORT(data,=20len);=0A+=09=09=09memcpy(data=20+=201,=20= VARDATA(DatumGetPointer(normalized[i])),=20len=20-=201);=0A+=0A+=09=09=09= formnewtup=20=3D=20true;=0A+=09=09=09normalized[i]=20=3D=20= PointerGetDatum(data);=0A+=09=09=09need_free[i]=20=3D=20true;=0A+=09=09}=0A= +=0A=20=09}=0A=20=0A-=09/*=20Easier=20case:=20Tuple=20has=20varlena=20= datums,=20none=20of=20which=20are=20compressed=20*/=0A+=09/*=0A+=09=20*=20= Easier=20case:=20Tuple=20has=20varlena=20datums,=20none=20of=20which=20= are=20compressed=20or=0A+=09=20*=20short=20with=204B=20header=0A+=09=20= */=0A=20=09if=20(!formnewtup)=0A=20=09=09return=20itup;=0A=20=0A@@=20= -2997,6=20+3019,10=20@@=20bt_normalize_tuple(BtreeCheckState=20*state,=20= IndexTuple=20itup)=0A=20=09=20*=20creating=20normalized=20version=20of=20= the=20tuple=20from=20uncompressed=20input=20datums=0A=20=09=20*=20= (normalized=20input=20datums).=20=20This=20is=20rather=20naive,=20but=20= shouldn't=20be=0A=20=09=20*=20necessary=20too=20often.=0A+=09=20*=20Also=20= tuple=20had=20short=20varlena=20datums=20with=204B=20header.=20Actually=20= there=20is=20no=0A+=09=20*=20restriction=20with=20have=20heap=20tuple=20= containing=20varlena=20datum=20with=204B=20header=0A+=09=20*=20and=20= corresponding=20index=20tuple=20containing=20varlena=20datum=20with=201B=20= header.=0A+=09=20*=20For=20fingerprinting=20let's=20convert=20heap=20= tuple=20varlena=20datum=20to=201B=20format.=0A=20=09=20*=0A=20=09=20*=20= Note=20that=20we=20rely=20on=20deterministic=20index_form_tuple()=20= TOAST=20compression=0A=20=09=20*=20of=20normalized=20input.=0A@@=20= -3006,7=20+3032,7=20@@=20bt_normalize_tuple(BtreeCheckState=20*state,=20= IndexTuple=20itup)=0A=20=0A=20=09/*=20Cannot=20leak=20memory=20here=20*/=0A= =20=09for=20(i=20=3D=200;=20i=20<=20tupleDescriptor->natts;=20i++)=0A-=09= =09if=20(toast_free[i])=0A+=09=09if=20(need_free[i])=0A=20=09=09=09= pfree(DatumGetPointer(normalized[i]));=0A=20=0A=20=09return=20reformed;=0A= --=20=0A2.37.1=20(Apple=20Git-137.1)=0A=0A= --Apple-Mail=_1355B393-CA22-494E-A3D2-53080FC2FFDD Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii --Apple-Mail=_1355B393-CA22-494E-A3D2-53080FC2FFDD--