Message-ID: <2006c427-e070-b58a-e6d5-6fefa9f1a268@gmail.com>
Date: Fri, 5 May 2023 06: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 #17920: Incorrect memory access in array_position(s) is
 detected (or not)
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org, Richard Guo <guofenglinux@gmail.com>
References: <17920-f7c228c627b6d02e@postgresql.org>
 <3414186.1683215628@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <3414186.1683215628@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/2006c427-e070-b58a-e6d5-6fefa9f1a268%40gmail.com>
Precedence: bulk

04.05.2023 18:53, Tom Lane wrote:
>> ==00:00:00:04.955 1823064==    at 0x5EDBD0: array_positions
>> (array_userfuncs.c:806)
> Fixed, thanks for the report!

Thank you for the fix!

>> I was surprised by the fact, that Valgrind doesn't complain for the single
>> query execution:
>> SELECT array_positions('{}'::int[], 1)
>> IIUC, this is explained by the lack of red zones around palloc'ed memory
>> areas.
>> sed "s/VALGRIND_CREATE_MEMPOOL(\(.*\), 0, false);/VALGRIND_CREATE_MEMPOOL(\1, 16, false);/" -i
>> src/backend/utils/mmgr/mcxt.c
>> on master (after 414d66220) helps Valgrind to detect the invalid read on
>> the single query execution too:
> I didn't try valgrind'ing this on HEAD, but I think this situation
> might have been improved by 414d66220.

Exactly. On 414d66220~1, with the redzones added
(VALGRIND_CREATE_MEMPOOL(..., 16, ...) [1]) even initdb fails:
running bootstrap script ... ==00:00:00:00.782 2770024== Invalid read of size 4
==00:00:00:00.782 2770024==    at 0x7729F8: GetMemoryChunkMethodID (mcxt.c:195)
==00:00:00:00.782 2770024==    by 0x7729F8: pfree (mcxt.c:1439)
==00:00:00:00.782 2770024==    by 0x3D7D8D: check_temp_tablespaces (tablespace.c:1304)
==00:00:00:00.782 2770024==    by 0x757492: call_string_check_hook (guc.c:6838)
==00:00:00:00.782 2770024==    by 0x7595B6: InitializeOneGUCOption (guc.c:1689)
==00:00:00:00.782 2770024==    by 0x75BC10: InitializeGUCOptions (guc.c:1520)
==00:00:00:00.782 2770024==    by 0x2CE4DD: BootstrapModeMain (bootstrap.c:215)
==00:00:00:00.782 2770024==    by 0x4694F3: main (main.c:189)
==00:00:00:00.782 2770024==  Address 0x73b7fd0 is 8 bytes before a block of size 1 client-defined
==00:00:00:00.782 2770024==    at 0x77186D: MemoryContextAlloc (mcxt.c:1035)
==00:00:00:00.782 2770024==    by 0x7731FB: MemoryContextStrdup (mcxt.c:1616)
==00:00:00:00.782 2770024==    by 0x773225: pstrdup (mcxt.c:1626)
==00:00:00:00.782 2770024==    by 0x3D7B46: check_temp_tablespaces (tablespace.c:1210)
==00:00:00:00.782 2770024==    by 0x757492: call_string_check_hook (guc.c:6838)
==00:00:00:00.782 2770024==    by 0x7595B6: InitializeOneGUCOption (guc.c:1689)
==00:00:00:00.782 2770024==    by 0x75BC10: InitializeGUCOptions (guc.c:1520)
==00:00:00:00.782 2770024==    by 0x2CE4DD: BootstrapModeMain (bootstrap.c:215)
==00:00:00:00.782 2770024==    by 0x4694F3: main (main.c:189)
==00:00:00:00.782 2770024==


But on HEAD `make check` passes for me.
So may be it makes sense to add the redzones for v16?/v17 to catch
such errors more reliably?
Though it's not clear to me, how many previously missed cases could be
detected with the redzones. Anyway, I'm going to perform tests with
this additional protection and share my findings.

[1] https://valgrind.org/docs/manual/mc-manual.html#mc-manual.mempools

Best regards,
Alexander