Re: [PATCH v3] refs: fix uninitialized memory access of `max_index`

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 24, 2025, 14:46 UTC
Message-ID: <Z5On1waE-2uwIjS2@pks.im>
In-Reply-To: <20250124140203.886324-1-karthik.188@gmail.com>

On Fri, Jan 24, 2025 at 03:02:03PM +0100, Karthik Nayak wrote:

Show 26 quoted lines

> When migrating reflogs between reference backends, maintaining the
> original order of the reflog entries is crucial. To achieve this, an
> `index` field is stored within the `ref_update` struct.
> 
> In the reftable backend, before writing any references, the writer must
> be configured with the minimum and maximum update index values. The
> `max_update_index` is derived from the maximum `ref_update.index` value
> in a transaction . The commit bc67b4ab5f (reftable: write correct
> max_update_index to header, 2025-01-15) addressed this by propagating the
> `max_update_index` value from the transaction to
> `write_transaction_table_arg` and, ultimately, to
> `reftable_writer_set_limits()`, which sets the min and max index for the
> reftable writer.
> 
> However, that commit introduced an issue:
> 
>   - In `reftable_transaction_data`, which contains an array of
>   `write_transaction_table_arg`, only the first element was assigned the
>   `max_index` value.
> 
> As a result, any elements beyond the first in the array contained
> uninitialized `max_index`. The writer contains multiple elements of
> `write_transaction_table_arg` to correspond to different worktrees being
> written. This uninitialized value was later used to set the
> `max_update_index` for the writer, potentially causing overflow or
> undefined behavior.

It reads a bit funny as a bulleted list with a single item, only. A suggestion for the above:

    However, we only set the update index for the first
    `write_transaction_table_arg`, even though there can be multiple
    such arguments. This is the case when we write to multiple stacks in
    a single transaction, e.g. when updating references in two different
    worktrees at once. And, if so, we wouldn't have initialized the
    update index for any but the first such argument. This uninitialized
    value was later used to set the `max_update_index` for the writer,
    potentially causing undefined behaviour.

Other than that this is nicely described, and the fix looks reasonable, too.

Patrick