Re: REPACK enhancements

shihao zhong <zhong950419(at)gmail(dot)com> wrote:

> 0001: in heap_insert_for_repack(), the reform slot borrows the source
> tuple's by-ref data (ExecForceStoreHeapTuple with shouldFree=false), but
> 'tuple' is freed before the insert reads from it -- a use-after-free if
> ExecFetchSlotHeapTuple(src) returns shouldFree=true. Latent today (the copy
> paths pass buffer/heaptuple slots), but the "nothing should depend on it
> now" comment is wrong. Suggest freeing after the insert; that also drops the
> slot-type assumption.

Since 'reform' slot is assumed to be "virtual", a new copy should be created
for the insert:

table_tuple_insert()
-> heapam_tuple_insert()
-> ExecFetchSlotHeapTuple(slot, materialize=true, ...)

('tuple' points to something that actually should not need freeing - added a
new assertion and comment.)

> Also, could you register this in the open commitfest so cfbot tracks it?

It's registered in the current CF. I think the "needs rebase" status is the
problem. Rebased version attached here.

> Testing 0004 now, will post new finding in following emails

Thanks!

--
Antonin Houska
Web: https://www.cybertec-postgresql.com