Re: btree_gist valgrind warnings about uninitialized memory

On 05/06/2014 10:03 PM, Andres Freund wrote:
> Hi,
>
> I am not sure whether these are new for 9.4 but they're worth looking at
> anyway:
> Valgrind was run with:
>   --trace-children=yes --quiet \
>   --track-origins=yes --leak-check=no \
>   --read-var-info=yes \
>   --suppressions=/home/andres/src/postgresql/src/tools/valgrind.supp \
>   <postgres with options>
> All seem to be due btree_bit's fault and they seem to have a common
> origin. Didn't look at code.
>
> ==27401== Conditional jump or move depends on uninitialised value(s)
> ==27401==    at 0x4C2C812: bcmp (mc_replace_strmem.c:935)
> ==27401==    by 0x8A8533: byteacmp (varlena.c:2735)
> ==27401==    by 0x8D70A2: DirectFunctionCall2Coll (fmgr.c:1050)
> ==27401==    by 0x180822C6: gbt_bitcmp (btree_bit.c:68)
> ==27401==    by 0x18079C05: gbt_vsrt_cmp (btree_utils_var.c:430)
> ==27401==    by 0x919F66: qsort_arg (qsort_arg.c:121)
> ==27401==    by 0x91A531: qsort_arg (qsort_arg.c:186)
> ==27401==    by 0x91A531: qsort_arg (qsort_arg.c:186)
> ==27401==    by 0x18079E68: gbt_var_picksplit (btree_utils_var.c:484)
> ==27401==    by 0x180825AC: gbt_bit_picksplit (btree_bit.c:180)
> ==27401==    by 0x8D7AD3: FunctionCall2Coll (fmgr.c:1324)
> ==27401==    by 0x497701: gistUserPicksplit (gistsplit.c:433)

In a btree_gist bit/varbit index, an internal index item  consists of a 
lower and upper key. The full Varbit struct is not stored as the 
lower/upper key, but only the "bits" array. The lower key is expanded to 
next INTALIGNed size, so that when the lower and upper keys are put 
together into one Datum, the length-field of the upper key is properly 
aligned.

The bug is in gbt_bit_xfrm(), which expands a VarBit struct to an 
INTALIGNed bytea, containing just the bits array. It doesn't initialize 
the padding bytes. That can confuse comparisons against lower/upper 
keys, as the comparison routine doesn't know which bytes are padding, 
and will merrily compare them as well. I was able to get wrong query 
results after modifying gbt_bit_xfrm() to knowingly initialize the 
padding bytes to random():

create table varbittest (b varbit);
insert into varbittest select '001'::varbit from generate_series(1, 100) 
union all select '01'::varbit from generate_series(1, 100) union all 
select '1'::varbit from generate_series(1, 100);
create index varbittest_idx on varbittest using gist(b);
set enable_seqscan=off;

-- should return 200
postgres=# select count(*) from varbittest where b >= '01'; count
-------   169
(1 row)


I committed a fix to gbt_bit_xfrm() to zero the padding bytes. However, 
if you have an existing btree_gist index on bit/varbit, you will have to 
reindex as there can be non-zero padding bytes on disk already. That 
should be mentioned in the release notes.

Thanks for the Valgrinding!

- Heikki