From e8c2a13c58e16a8482fb1a75f5d99754babaab9f Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Thu, 25 Apr 2024 10:55:20 +0900 Subject: [PATCH] radixtree: Fix SIGSEGV at update of embeddable value to non-embeddable. Also, fix a memory leak when updating from non-embeddable to embeddable. Both were unreachable without adding C code. Reported-by: Noah Misch Author: Noah Misch Reviewed-by: Masahiko Sawada Discussion: https://postgr.es/m/20240424210319.4c.nmisch%40google.com --- src/include/lib/radixtree.h | 6 +- .../test_tidstore/expected/test_tidstore.out | 112 ++++++++++++++++++ .../test_tidstore/sql/test_tidstore.sql | 16 +++ .../modules/test_tidstore/test_tidstore.c | 15 +++ 4 files changed, 148 insertions(+), 1 deletion(-) diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h index d9f545d491..2896a6efc5 100644 --- a/src/include/lib/radixtree.h +++ b/src/include/lib/radixtree.h @@ -1749,6 +1749,10 @@ have_slot: if (RT_VALUE_IS_EMBEDDABLE(value_p)) { + /* free the existing leaf */ + if (found && !RT_CHILDPTR_IS_VALUE(*slot)) + RT_FREE_LEAF(tree, *slot); + /* store value directly in child pointer slot */ memcpy(slot, value_p, value_sz); @@ -1765,7 +1769,7 @@ have_slot: { RT_CHILD_PTR leaf; - if (found) + if (found && !RT_CHILDPTR_IS_VALUE(*slot)) { Assert(RT_PTR_ALLOC_IS_VALID(*slot)); leaf.alloc = *slot; diff --git a/src/test/modules/test_tidstore/expected/test_tidstore.out b/src/test/modules/test_tidstore/expected/test_tidstore.out index 06c610e84c..2ef701f866 100644 --- a/src/test/modules/test_tidstore/expected/test_tidstore.out +++ b/src/test/modules/test_tidstore/expected/test_tidstore.out @@ -79,6 +79,118 @@ SELECT test_destroy(); (1 row) +-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions +SELECT test_create(false); + test_create +------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2,3,4,100]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets(); + do_set_block_offsets +---------------------- + 1 +(1 row) + + check_set_block_offsets +------------------------- + +(1 row) + +SELECT test_destroy(); + test_destroy +-------------- + +(1 row) + -- Use shared memory this time. We can't do that in test_radixtree.sql, -- because unused static functions would raise warnings there. SELECT test_create(true); diff --git a/src/test/modules/test_tidstore/sql/test_tidstore.sql b/src/test/modules/test_tidstore/sql/test_tidstore.sql index bb31877b9a..9ce1119128 100644 --- a/src/test/modules/test_tidstore/sql/test_tidstore.sql +++ b/src/test/modules/test_tidstore/sql/test_tidstore.sql @@ -50,6 +50,22 @@ SELECT test_is_full(); -- Re-create the TID store for randommized tests. SELECT test_destroy(); + + +-- Test replacements crossing RT_CHILDPTR_IS_VALUE in both directions +SELECT test_create(false); +SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2,3,4,100]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2,3,4]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2,3]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1,2]::int2[]); SELECT check_set_block_offsets(); +SELECT do_set_block_offsets(1, array[1]::int2[]); SELECT check_set_block_offsets(); +SELECT test_destroy(); + + -- Use shared memory this time. We can't do that in test_radixtree.sql, -- because unused static functions would raise warnings there. SELECT test_create(true); diff --git a/src/test/modules/test_tidstore/test_tidstore.c b/src/test/modules/test_tidstore/test_tidstore.c index 0a3a58722d..5417163407 100644 --- a/src/test/modules/test_tidstore/test_tidstore.c +++ b/src/test/modules/test_tidstore/test_tidstore.c @@ -146,6 +146,18 @@ sanity_check_array(ArrayType *ta) errmsg("argument must be empty or one-dimensional array"))); } +static void +purge_from_verification_array(BlockNumber blkno) +{ + int dst = 0; + + for (int src = 0; src < items.num_tids; src++) + if (ItemPointerGetBlockNumber(&items.insert_tids[src]) != blkno) + items.insert_tids[dst++] = items.insert_tids[src]; + items.num_tids = dst; +} + + /* Set the given block and offsets pairs */ Datum do_set_block_offsets(PG_FUNCTION_ARGS) @@ -165,6 +177,9 @@ do_set_block_offsets(PG_FUNCTION_ARGS) TidStoreSetBlockOffsets(tidstore, blkno, offs, noffs); TidStoreUnlock(tidstore); + /* Remove the existing items of blkno from the verification array */ + purge_from_verification_array(blkno); + /* Set TIDs in verification array */ for (int i = 0; i < noffs; i++) { -- 2.39.3