Re: BUG #18483: Segmentation fault in tests modules

On Sun, Jun 09, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote:
> 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.

Interesting, I cannot see this one, did you use a specific option with
valgrind?

> 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
>
> I guess, test_is_full() is missing a check-for-null too.

Yeah, this one is simple enough that it should be fixed. Even if it
is not in the scope of the test, that's a bit surprising. An
undefined tid store means that it should not be full, but returning an
error may make more sense if there's nothing.

> 4)
> 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)

Fun. True enough that this function is only here for toastable
values, so that makes sense.
--
Michael