Patrick Steinhardt <ps@xxxxxx> writes: > On Fri, Jan 24, 2025 at 03:02:03PM +0100, Karthik Nayak wrote: >> 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 Thanks Patrick for taking the time, this seems much better. Let me add this in for the next version.
Attachment:
signature.asc
Description: PGP signature
