> On Dec 11, 2025, at 22:59, Melanie Plageman <[email protected]> wrote:
>
> On Wed, Dec 10, 2025 at 11:02 PM Chao Li <[email protected]> wrote:
>>
>> While reviewing Melanie's patch [1], I found this bug where presult is not
>> initialized. Let me explain the logic.
>
> Thanks for looking closely at the code.
>
>> static int
>> lazy_scan_prune(LVRelState *vacrel,
>> presult, &prstate); <== immediately pass uninitialized presult to
>> prune_freeze_setup
>>
>> Then in prune_freeze_setup():
>>
>> prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <==
>> presult->deadoffsets could be a random value
>>
>> Attached is a simple fix by just initializing presult in the first place
>> with {0}.
>
> I don't think zero-initializing deadoffsets is needed. We don't read
> offsets from it in heap_page_prune_and_freeze() -- it's a result
> variable. We only set offsets in it (see heap_prune_record_dead()).
> And because we track exactly how many are initialized in
> prstate->lpdead_items, the caller (lazy_scan_heap() via
> lazy_scan_prune()) will only access those dead offsets which have been
> initialized. I think this is a pretty common pattern in C. We don't
> zero-initialize the other arrays of offsets in the PruneState
> (redirected, dead, etc) and, for example, lazy_scan_noprune() doesn't
> zero-initialize the deadoffsets array that it fills in.
Thanks for the explanation. I didn’t notice deadoffsets is an array, so
prune_freeze_setup() only assign the array address to prstate, which doesn’t
care about content stored in the array. In this case, initializing presult is
not required.
>
> The reason PruneFreezeResult is passed into prune_freeze_setup() is
> that we save a pointer to the deadoffsets array in the PruneState
> instead of having a copy of the whole array (to save stack space and
> effort copying the array from PruneState into PruneFreezeResult at the
> end).
>
> Other than that, we wait to initialize PruneFreezeResult's members
> until the end of heap_page_prune_and_freeze() to make it clear that we
> are actually setting all the members. If we filled them out throughout
> the various functions and helpers, it would be less clear that we have
> filled in all the return values.
I don’t get this point. presult is a local variable defined in the caller
function, filling with random values, there is no way to distinct if a field
has been set or not because of random values. From this perspective, zero-out
presult may make it clear that we are actually setting the members.
>
> I could add a comment to prune_freeze_setup() where we save the
> deadoffsets pointer that explains why we are doing that instead of
> just having a deadoffsets array in the PruneState. Would that help
> with the confusion?
>
That will be great.
From “clearly knowing which members are set” perspective, I still feel
initializing presult = {0} is useful, at least harmless. There are only 2
places, not a big change.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
