From: Karthik Nayak <karthik.188@gmail•com>
To: gitster@pobox•com
Cc: Johannes.Schindelin@gmx•de, git@vger•kernel.org,
karthik.188@gmail•com, ps@pks•im, sandals@crustytoothpaste•net
Subject: [PATCH v3] refs: fix uninitialized memory access of `max_index`
Date: Fri, 24 Jan 2025 15:02:03 +0100 [thread overview]
Message-ID: <20250124140203.886324-1-karthik.188@gmail.com> (raw)
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
next prev parent reply other threads:[~2025-01-24 14:02 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-13 13:56 Bug in 2.48 with `git refs migrate` brian m. carlson
2025-01-13 15:45 ` Patrick Steinhardt
2025-01-15 11:54 ` Karthik Nayak
2025-01-15 17:07 ` Junio C Hamano
2025-01-16 6:44 ` Karthik Nayak
2025-01-16 2:05 ` brian m. carlson
2025-01-16 7:29 ` Karthik Nayak
2025-01-16 23:21 ` brian m. carlson
2025-01-17 0:04 ` Junio C Hamano
2025-01-17 6:17 ` Karthik Nayak
2025-01-17 6:15 ` Karthik Nayak
2025-01-23 13:56 ` [PATCH v2] reftable: write correct max_update_index to header Karthik Nayak
2025-01-23 18:11 ` Junio C Hamano
2025-01-23 18:24 ` Junio C Hamano
2025-01-24 14:02 ` Karthik Nayak [this message]
2025-01-24 14:46 ` [PATCH v3] refs: fix uninitialized memory access of `max_index` Patrick Steinhardt
2025-01-24 15:48 ` Karthik Nayak
2025-01-27 10:20 ` Patrick Steinhardt
2025-01-27 16:24 ` Junio C Hamano
2025-01-24 4:06 ` [PATCH v2] reftable: write correct max_update_index to header Karthik Nayak
2025-01-24 15:25 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250124140203.886324-1-karthik.188@gmail.com \
--to=karthik.188@gmail$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=ps@pks$(echo .)im \
--cc=sandals@crustytoothpaste$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox