[PATCH v3] refs: fix uninitialized memory access of `max_index`
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jan 24, 2025, 14:02 UTC
- Message-ID
- <20250124140203.886324-1-karthik.188@gmail.com>
- In-Reply-To
- <xmqq5xm5s80e.fsf@gitster.g>
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.
Fix this by:
- Initializing the `max_index` field to 0. - Moving the assignment of `max_index` in `reftable_be_transaction_finish()` inside the loop, ensuring all elements of the array are correctly initialized.
Initializing `max_index` to `0` is not strictly necessary, as all elements of `write_transaction_table_arg.max_index` are now assigned correctly. However, this initialization is added for consistency and to safeguard against potential future changes that might inadvertently introduce uninitialized memory access.
Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- Hello,
As suggested, I've redone my patch to make this a relative patch on top of 'kn/reflog-migration-fix'.
This is based on top of maint with 'kn/reflog-migration-fix' merged in.
Thanks --- refs/reftable-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 289496058e..d39a14c5a4 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1020,6 +1020,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, arg->updates_nr = 0; arg->updates_alloc = 0; arg->updates_expected = 0; + arg->max_index = 0; } arg->updates_expected++; @@ -1628,10 +1629,9 @@ static int reftable_be_transaction_finish(struct ref_store *ref_store UNUSED, struct reftable_transaction_data *tx_data = transaction->backend_data; int ret = 0; - if (tx_data->args) - tx_data->args->max_index = transaction->max_index; - for (size_t i = 0; i < tx_data->args_nr; i++) { + tx_data->args[i].max_index = transaction->max_index; + ret = reftable_addition_add(tx_data->args[i].addition, write_transaction_table, &tx_data->args[i]); if (ret < 0)
-- 2.47.0