Обсуждение: BUG #17920: Incorrect memory access in array_position(s) is detected (or not)

The following bug has been logged on the website:

Bug reference:      17920
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

When the following script:
SELECT array_positions('{}'::int[], 1); SELECT array_positions('{}'::int[],
1)
is executed under Valgrind, an incorrect memory access is detected:
==00:00:00:04.955 1823064== Invalid read of size 4
==00:00:00:04.955 1823064==    at 0x5EDBD0: array_positions
(array_userfuncs.c:806)
==00:00:00:04.955 1823064==    by 0x401A83: ExecInterpExpr
(execExprInterp.c:727)
==00:00:00:04.955 1823064==    by 0x3FE294: ExecInterpExprStillValid
(execExprInterp.c:1826)
==00:00:00:04.955 1823064==    by 0x4EFD58: ExecEvalExprSwitchContext
(executor.h:341)
==00:00:00:04.955 1823064==    by 0x4EFD58: evaluate_expr (clauses.c:4823)
==00:00:00:04.955 1823064==    by 0x4EFF34: evaluate_function
(clauses.c:4325)
==00:00:00:04.955 1823064==    by 0x4F23EC: simplify_function
(clauses.c:3908)
==00:00:00:04.955 1823064==    by 0x4F01F8: eval_const_expressions_mutator
(clauses.c:2427)
==00:00:00:04.955 1823064==    by 0x480F71: expression_tree_mutator
(nodeFuncs.c:3080)
==00:00:00:04.955 1823064==    by 0x4F1632: eval_const_expressions_mutator
(clauses.c:3527)
==00:00:00:04.955 1823064==    by 0x4811BB: expression_tree_mutator
(nodeFuncs.c:3166)
==00:00:00:04.955 1823064==    by 0x4F1632: eval_const_expressions_mutator
(clauses.c:3527)
==00:00:00:04.955 1823064==    by 0x4F17AD: eval_const_expressions
(clauses.c:2107)
==00:00:00:04.955 1823064==  Address 0x73bff40 is 6,736 bytes inside a
recently re-allocated block of size 8,192 alloc'd
==00:00:00:04.955 1823064==    at 0x4848899: malloc
(vg_replace_malloc.c:381)
==00:00:00:04.955 1823064==    by 0x73ACE8: AllocSetContextCreateInternal
(aset.c:469)
==00:00:00:04.955 1823064==    by 0x522DE8: PostmasterMain
(postmaster.c:612)
==00:00:00:04.955 1823064==    by 0x467E6B: main (main.c:202)
==00:00:00:04.955 1823064==

The same issue observed with array_position().
(Discovered during testing of new array_in() implementation.)

Here, array_positions() tries to get a lower bound of the (non-existing)
first array dimension:
    position = (ARR_LBOUND(array))[0] - 1;

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:
2023-05-04 13:42:36.125 MSK [1841879] LOG:  statement: SELECT
array_positions('{}'::int[], 1)
==00:00:00:04.271 1841879== Invalid read of size 4
==00:00:00:04.271 1841879==    at 0x61955F: array_positions
(array_userfuncs.c:1430)
==00:00:00:04.271 1841879==    by 0x40481F: ExecInterpExpr
(execExprInterp.c:733)
==00:00:00:04.271 1841879==    by 0x400E3D: ExecInterpExprStillValid
(execExprInterp.c:1858)
==00:00:00:04.271 1841879==    by 0x5171FA: ExecEvalExprSwitchContext
(executor.h:354)
==00:00:00:04.271 1841879==    by 0x5171FA: evaluate_expr (clauses.c:4890)
==00:00:00:04.271 1841879==    by 0x5173D6: evaluate_function
(clauses.c:4397)
==00:00:00:04.271 1841879==    by 0x519793: simplify_function
(clauses.c:3985)
==00:00:00:04.271 1841879==    by 0x5176E9: eval_const_expressions_mutator
(clauses.c:2495)
==00:00:00:04.271 1841879==    by 0x484CF2: expression_tree_mutator_impl
(nodeFuncs.c:3277)
==00:00:00:04.271 1841879==    by 0x517493: eval_const_expressions_mutator
(clauses.c:3604)
==00:00:00:04.271 1841879==    by 0x484EF6: expression_tree_mutator_impl
(nodeFuncs.c:3363)
==00:00:00:04.271 1841879==    by 0x517493: eval_const_expressions_mutator
(clauses.c:3604)
==00:00:00:04.271 1841879==    by 0x518C08: eval_const_expressions
(clauses.c:2175)
==00:00:00:04.271 1841879==  Address 0x73bfb30 is 0 bytes after a block of
size 16 client-defined

PG Bug reporting form <noreply@postgresql.org> writes:
> When the following script:
> SELECT array_positions('{}'::int[], 1); SELECT array_positions('{}'::int[],
> 1)
> is executed under Valgrind, an incorrect memory access is detected:
> ==00:00:00:04.955 1823064== Invalid read of size 4

> ==00:00:00:04.955 1823064==    at 0x5EDBD0: array_positions
> (array_userfuncs.c:806)

Fixed, thanks for the report!

> 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.

            regards, tom lane

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