From fd49e8f32325c675d9bb6e26fcdbe9754249932f Mon Sep 17 00:00:00 2001 From: John Naylor Date: Fri, 21 Jun 2024 14:59:11 +0700 Subject: [PATCH] Prevent access of uninitialized memory in radix tree nodes RT_NODE_16_SEARCH_EQ() performs comparisions using vector registers on x64-64 and aarch64. We apply a mask to the resulting bitfield to eliminate irrelevant bits that may be set. This ensures correct behavior, but Valgrind complains of the partially-uninitialised values. So far the warnings have only occurred on aarch64, which explains why this hasn't been seen earlier. To fix this warning, initialize the whole fixed-sized part of the nodes upon allocation, rather than just do the minimum initialization to function correctly. The initialization for node48 is a bit different in that the 256-byte slot index array must be populated with "invalid index" rather than zero. Experimentation has shown that compilers tend to emit code that uselessly memsets that array twice. To avoid pessimizing this path, swap the order of the slot_idxs[] and isset[] arrays so we can initialize with two non-overlapping memset calls. Reported by Tomas Vondra Analysis and patch by Tom Lane, reviewed by Masahiko Sawada. I investigated the behavior of memset calls to overlapping regions, leading to the above tweaks to node48 as discussed in the thread. Discussion: https://postgr.es/m/120c63ad-3d12-415f-a7bf-3da451c31bf6%40enterprisedb.com --- src/include/lib/radixtree.h | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h index 338e1d741d..88bf695e3f 100644 --- a/src/include/lib/radixtree.h +++ b/src/include/lib/radixtree.h @@ -541,15 +541,19 @@ typedef struct RT_NODE_48 { RT_NODE base; - /* The index of slots for each fanout */ + /* bitmap to track which slots are in use */ + bitmapword isset[RT_BM_IDX(RT_FANOUT_48_MAX)]; + + /* + * Lookup table for indexes into the children[] array. We make this the + * last fixed-size member so that it's convenient to memset separately + * from the previous members. + */ uint8 slot_idxs[RT_NODE_MAX_SLOTS]; /* Invalid index */ #define RT_INVALID_SLOT_IDX 0xFF - /* bitmap to track which slots are in use */ - bitmapword isset[RT_BM_IDX(RT_FANOUT_48_MAX)]; - /* number of children depends on size class */ RT_PTR_ALLOC children[FLEXIBLE_ARRAY_MEMBER]; } RT_NODE_48; @@ -845,27 +849,25 @@ RT_ALLOC_NODE(RT_RADIX_TREE * tree, const uint8 kind, const RT_SIZE_CLASS size_c /* initialize contents */ - memset(node, 0, sizeof(RT_NODE)); switch (kind) { case RT_NODE_KIND_4: + memset(node, 0, offsetof(RT_NODE_4, children)); + break; case RT_NODE_KIND_16: + memset(node, 0, offsetof(RT_NODE_16, children)); break; case RT_NODE_KIND_48: { RT_NODE_48 *n48 = (RT_NODE_48 *) node; - memset(n48->isset, 0, sizeof(n48->isset)); + memset(n48, 0, offsetof(RT_NODE_48, slot_idxs)); memset(n48->slot_idxs, RT_INVALID_SLOT_IDX, sizeof(n48->slot_idxs)); break; } case RT_NODE_KIND_256: - { - RT_NODE_256 *n256 = (RT_NODE_256 *) node; - - memset(n256->isset, 0, sizeof(n256->isset)); - break; - } + memset(node, 0, offsetof(RT_NODE_256, children)); + break; default: pg_unreachable(); }