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

diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 338e1d741d4e45d60819c68aa4965a5f698a3aa1..88bf695e3f354f5bdfcc8d8dbafe87dce0e4d35e 100644 (file)
--- 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();
    }