Thread: btree_gist valgrind warnings about uninitialized memory

btree_gist valgrind warnings about uninitialized memory

From
Andres Freund
Date:
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)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 
==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x8A853B: byteacmp (varlena.c:2736)
==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)
==27401==    by 0x498491: gistSplitByKey (gistsplit.c:697)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 
==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x18079C0D: gbt_vsrt_cmp (btree_utils_var.c:431)
==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)
==27401==    by 0x498491: gistSplitByKey (gistsplit.c:697)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 
==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x919F69: 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)
==27401==    by 0x498491: gistSplitByKey (gistsplit.c:697)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 
==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x8A853B: byteacmp (varlena.c:2736)
==27401==    by 0x8D70A2: DirectFunctionCall2Coll (fmgr.c:1050)
==27401==    by 0x180822C6: gbt_bitcmp (btree_bit.c:68)
==27401==    by 0x180794CA: gbt_var_bin_union (btree_utils_var.c:249)
==27401==    by 0x18079EC2: gbt_var_picksplit (btree_utils_var.c:496)
==27401==    by 0x180825AC: gbt_bit_picksplit (btree_bit.c:180)
==27401==    by 0x8D7AD3: FunctionCall2Coll (fmgr.c:1324)
==27401==    by 0x497701: gistUserPicksplit (gistsplit.c:433)
==27401==    by 0x498491: gistSplitByKey (gistsplit.c:697)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 
==27401== Conditional jump or move depends on uninitialised value(s)
==27401==    at 0x180794CD: gbt_var_bin_union (btree_utils_var.c:249)
==27401==    by 0x18079EC2: gbt_var_picksplit (btree_utils_var.c:496)
==27401==    by 0x180825AC: gbt_bit_picksplit (btree_bit.c:180)
==27401==    by 0x8D7AD3: FunctionCall2Coll (fmgr.c:1324)
==27401==    by 0x497701: gistUserPicksplit (gistsplit.c:433)
==27401==    by 0x498491: gistSplitByKey (gistsplit.c:697)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401==    by 0x499324: gistBuildCallback (gistbuild.c:490)
==27401==  Uninitialised value was created by a heap allocation
==27401==    at 0x4C274A0: malloc (vg_replace_malloc.c:291)
==27401==    by 0x8F8EE1: AllocSetAlloc (aset.c:853)
==27401==    by 0x8FAA71: palloc (mcxt.c:677)
==27401==    by 0x18078E37: gbt_var_decompress (btree_utils_var.c:42)
==27401==    by 0x8D79E5: FunctionCall1Coll (fmgr.c:1298)
==27401==    by 0x48E4A6: gistdentryinit (gistutil.c:549)
==27401==    by 0x498217: gistSplitByKey (gistsplit.c:644)
==27401==    by 0x48BEEC: gistSplit (gist.c:1270)
==27401==    by 0x48899D: gistplacetopage (gist.c:242)
==27401==    by 0x48BAEE: gistinserttuples (gist.c:1134)
==27401==    by 0x48BA73: gistinserttuple (gist.c:1093)
==27401==    by 0x48A71F: gistdoinsert (gist.c:750)
==27401== 


Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: btree_gist valgrind warnings about uninitialized memory

From
Heikki Linnakangas
Date:
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



Re: btree_gist valgrind warnings about uninitialized memory

From
Andres Freund
Date:
On 2014-05-13 15:33:08 +0300, Heikki Linnakangas wrote:
> 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.

> I committed a fix to gbt_bit_xfrm() to zero the padding bytes.

Thanks!

> Thanks for the Valgrinding!

That leaves the spgist thing and some infrastructure till we can setup a
valgrind animal... From what we've caught with it so far that seems
rather worthwile.
What's your plans with your spgist fix? Commit it once 9.5 is branched?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: btree_gist valgrind warnings about uninitialized memory

From
Heikki Linnakangas
Date:
On 05/13/2014 05:13 PM, Andres Freund wrote:
> That leaves the spgist thing and some infrastructure till we can setup a
> valgrind animal... From what we've caught with it so far that seems
> rather worthwile.
> What's your plans with your spgist fix? Commit it once 9.5 is branched?

Good question. I don't know. I would still like to commit it to 9.4. It 
doesn't require catalog changes, but it's an incompatible change in the 
WAL record format. If we commit it to 9.4, it means that you cannot 
replicate between 9.4beta1 and 9.4beta2. I think that's OK, but how do 
others feel about that?

- Heikki



Re: btree_gist valgrind warnings about uninitialized memory

From
Andres Freund
Date:
On 2014-05-14 14:55:38 +0300, Heikki Linnakangas wrote:
> On 05/13/2014 05:13 PM, Andres Freund wrote:
> >That leaves the spgist thing and some infrastructure till we can setup a
> >valgrind animal... From what we've caught with it so far that seems
> >rather worthwile.
> >What's your plans with your spgist fix? Commit it once 9.5 is branched?
> 
> Good question. I don't know. I would still like to commit it to 9.4. It
> doesn't require catalog changes, but it's an incompatible change in the WAL
> record format. If we commit it to 9.4, it means that you cannot replicate
> between 9.4beta1 and 9.4beta2. I think that's OK, but how do others feel
> about that?

I personally think that's not too bad.

Alternatively we could come up with a second way that's not breaking the
WAL format. I'd feel better if we had a back patchable fix...

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: btree_gist valgrind warnings about uninitialized memory

From
Tom Lane
Date:
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 05/13/2014 05:13 PM, Andres Freund wrote:
>> What's your plans with your spgist fix? Commit it once 9.5 is branched?

> Good question. I don't know. I would still like to commit it to 9.4. It 
> doesn't require catalog changes, but it's an incompatible change in the 
> WAL record format. If we commit it to 9.4, it means that you cannot 
> replicate between 9.4beta1 and 9.4beta2. I think that's OK, but how do 
> others feel about that?

I think that's an OK restriction as long as we warn people about it
(you could update a replication pair as long as you shut them both
down cleanly at the same time, right?).  Can the WAL replay routine
be made to detect incompatible records?

What worries me more is that post-beta1 fixes will, by definition,
get noticeably less beta testing than anything that went out in beta1.
So how confident are you in this fix?  Is it something we'd consider
back-patching in the absence of a WAL-format issue?
        regards, tom lane



Re: btree_gist valgrind warnings about uninitialized memory

From
Andres Freund
Date:
On 2014-05-14 10:07:18 -0400, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> > On 05/13/2014 05:13 PM, Andres Freund wrote:
> >> What's your plans with your spgist fix? Commit it once 9.5 is branched?
> 
> > Good question. I don't know. I would still like to commit it to 9.4. It 
> > doesn't require catalog changes, but it's an incompatible change in the 
> > WAL record format. If we commit it to 9.4, it means that you cannot 
> > replicate between 9.4beta1 and 9.4beta2. I think that's OK, but how do 
> > others feel about that?
> 
> I think that's an OK restriction as long as we warn people about it
> (you could update a replication pair as long as you shut them both
> down cleanly at the same time, right?).  Can the WAL replay routine
> be made to detect incompatible records?

We could just bump the wal version. Somewhat surprisingly that works if
both nodes are shutdown cleanly (primary first)... But the errors about
it are really ugly (will moan about unusable checkpoints), so it's
probably not a good idea. Especially as it'll make it an issue for all
users, not just the ones creating spgist indexes.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: btree_gist valgrind warnings about uninitialized memory

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-05-14 10:07:18 -0400, Tom Lane wrote:
>> I think that's an OK restriction as long as we warn people about it
>> (you could update a replication pair as long as you shut them both
>> down cleanly at the same time, right?).  Can the WAL replay routine
>> be made to detect incompatible records?

> We could just bump the wal version. Somewhat surprisingly that works if
> both nodes are shutdown cleanly (primary first)... But the errors about
> it are really ugly (will moan about unusable checkpoints), so it's
> probably not a good idea. Especially as it'll make it an issue for all
> users, not just the ones creating spgist indexes.

Yeah, I don't think we want to bump the WAL version code post-beta1.

Probably better to assign the modified spgist record a new xl_info ID
number, so that a beta1 slave would throw an error for it.
        regards, tom lane



Re: btree_gist valgrind warnings about uninitialized memory

From
Andres Freund
Date:
On 2014-05-14 12:20:55 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-05-14 10:07:18 -0400, Tom Lane wrote:
> >> I think that's an OK restriction as long as we warn people about it
> >> (you could update a replication pair as long as you shut them both
> >> down cleanly at the same time, right?).  Can the WAL replay routine
> >> be made to detect incompatible records?
> 
> > We could just bump the wal version. Somewhat surprisingly that works if
> > both nodes are shutdown cleanly (primary first)... But the errors about
> > it are really ugly (will moan about unusable checkpoints), so it's
> > probably not a good idea. Especially as it'll make it an issue for all
> > users, not just the ones creating spgist indexes.
> 
> Yeah, I don't think we want to bump the WAL version code post-beta1.
> 
> Probably better to assign the modified spgist record a new xl_info ID
> number, so that a beta1 slave would throw an error for it.

Since that ship has now sailed...? It's imo bad form to release a new
version that overwrites the stack and heap, even if we can't see a
concrete danger.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: btree_gist valgrind warnings about uninitialized memory

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-05-14 12:20:55 -0400, Tom Lane wrote:
>> Yeah, I don't think we want to bump the WAL version code post-beta1.
>> 
>> Probably better to assign the modified spgist record a new xl_info ID
>> number, so that a beta1 slave would throw an error for it.

> Since that ship has now sailed...? It's imo bad form to release a new
> version that overwrites the stack and heap, even if we can't see a
> concrete danger.

Yeah, no longer much reason to avoid changing the WAL version code.
        regards, tom lane