Message-ID: <250a21e5-d677-6b2a-2692-cd4233785e37@gmail.com>
Date: Sun, 9 Jun 2024 15:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #18483: Segmentation fault in tests modules
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, a.kozhemyakin@postgrespro.ru,
 pgsql-bugs@lists.postgresql.org
References: <18483-30bfff42de238000@postgresql.org>
 <Zl5GS6SHBXRAkl_c@paquier.xyz> <219049.1717792376@sss.pgh.pa.us>
 <ZmOJ3jfeltZQnN_M@paquier.xyz>
 <439c9310-1d71-f2c6-b213-a9a2ea732ba9@gmail.com>
 <336833.1717859243@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <336833.1717859243@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/250a21e5-d677-6b2a-2692-cd4233785e37%40gmail.com>
Precedence: bulk

Hello Tom,

08.06.2024 18:07, Tom Lane wrote:
> Alexander Lakhin <exclusion@gmail.com> writes:
>> May I ask you about the project policy regarding such test modules
>> failures?
>> Do you think all of these should be fixed or it's ok to have some
>> server-crashing modules in the tree?
> I don't think there's a one-size-fits-all policy for them.  Since
> we don't intend these modules to be installed, they don't really
> need to work in any cases beyond the ones exercised in the tests.
> Still, if it's easy to stop a misbehavior then we probably should.
> The test_predtest problem was just a missing check-for-null so
> I definitely regard that as worth fixing.  These other ones would
> have to be investigated enough to figure out the size of the fix
> before I'd venture an opinion.

Thank you for the explanation!

Then maybe you would like to take a look at the rest of my collection and
determine which sizes fit these issues:

2)
echo "
select test_enc_conversion('\x8bc68bcf8b', 'gb18030', 'gb18030', false);
" >> src/test/regress/sql/conversion.sql
TESTS="conversion" make -s check-tests

produces (under Valgrind):
==00:00:00:05.947 3320530== Invalid read of size 1
==00:00:00:05.947 3320530==    at 0x6F21E5: pg_gb18030_mblen (wchar.c:1000)
==00:00:00:05.947 3320530==    by 0x6F2BA0: pg_encoding_mblen (wchar.c:2072)
==00:00:00:05.947 3320530==    by 0x6B4EAF: report_invalid_encoding (mbutils.c:1700)
==00:00:00:05.947 3320530==    by 0x4867D3F: test_enc_conversion (regress.c:1154)
==00:00:00:05.947 3320530==    by 0x3B9796: ExecInterpExpr (execExprInterp.c:764)

If I understand correctly, the defect is inside test_enc_conversion(), not
in the core code.

3)
echo "
CREATE EXTENSION test_tidstore;
SELECT test_is_full();
" >src/test/modules/test_tidstore/sql/test_tidstore.sql
make -s check -C src/test/modules/test_tidstore

leads to:
Program terminated with signal SIGSEGV, Segmentation fault.
#0  TidStoreMemoryUsage (ts=0x0) at tidstore.c:553
553             if (TidStoreIsShared(ts))
(gdb) bt
#0  TidStoreMemoryUsage (ts=0x0) at tidstore.c:553
#1  0x00007f2d9c715cb7 in test_is_full (fcinfo=<optimized out>) at test_tidstore.c:308
#2  0x000055707450a302 in ExecInterpExpr (state=0x557074faecc8, econtext=0x557074faea98, isnull=0x7ffdd50b2747)
     at execExprInterp.c:740
#3  0x0000557074506c49 in ExecInterpExprStillValid (state=0x557074faecc8, econtext=0x557074faea98,
     isNull=0x7ffdd50b2747) at execExprInterp.c:1915

I guess, test_is_full() is missing a check-for-null too.

4)
echo "
CREATE TABLE tov(id int not null, ov oidvector);
INSERT INTO tov (id, ov) VALUES (1, '1 2 3');
SELECT id, make_tuple_indirect(tov)::text FROM tov;
" >> src/test/regress/sql/indirect_toast.sql
TESTS="indirect_toast" make -s check-tests

produces:
ERROR:  invalid memory alloc request size 18446744073608493557

I guess, make_tuple_indirect() could be improved with:
                 /* only work on existing, not-null varlenas */
                 if (TupleDescAttr(tupdesc, i)->attisdropped ||
                         nulls[i] ||
-                       TupleDescAttr(tupdesc, i)->attlen != -1)
+                       TupleDescAttr(tupdesc, i)->attlen != -1 ||
+                       TupleDescAttr(tupdesc, i)->attstorage == TYPSTORAGE_PLAIN)

Best regards,
Alexander