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