| Message ID | 20250124140203.886324-1-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f11f0a5a2db955f68776ea95aec42df7fcb8ce1b |
| Headers | show |
| Series | [v3] refs: fix uninitialized memory access of `max_index` | expand |
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
Patrick Steinhardt <ps@pks.im> 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.
On Fri, Jan 24, 2025 at 07:48:43AM -0800, Karthik Nayak wrote: > Thanks Patrick for taking the time, this seems much better. Let me add > this in for the next version. I've sent a v4 of this patch, but forgot to set the In-reply-to header. The patch can be found at [1]. Patrick [1]: <b7e3dd3cc870024f0e80dad26c5a7a96483c6cf4.1737970803.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Jan 24, 2025 at 07:48:43AM -0800, Karthik Nayak wrote: >> Thanks Patrick for taking the time, this seems much better. Let me add >> this in for the next version. > > I've sent a v4 of this patch, but forgot to set the In-reply-to header. > The patch can be found at [1]. > > Patrick > > [1]: <b7e3dd3cc870024f0e80dad26c5a7a96483c6cf4.1737970803.git.ps@pks.im> Ah, how very considerate of you. List-archive spelunkers have bidirectional links to see (1) from an older discussion, how the issues were resolved, and (2) from the final attempt, where the issues came from. Thanks.
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)
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(-)