Обсуждение: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy

On Wed, Dec 10, 2025 at 11:02 PM Chao Li <li.evan.chao@gmail.com> 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.

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 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?

- Melanie

> On Dec 11, 2025, at 22:59, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Wed, Dec 10, 2025 at 11:02 PM Chao Li <li.evan.chao@gmail.com> 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
addressto 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
isno way to distinct if a field has been set or not because of random values. From this perspective, zero-out presult
maymake 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/