From: Patrick Steinhardt <ps@pks•im>
To: Tsahi Elkayam <Tsahi.Elkayam@protonmail•com>
Cc: "git@vger•kernel.org" <git@vger•kernel.org>
Subject: Re: [PATCH v2] reftable/iter: fix UB in indexed_table_ref_iter_next
Date: Fri, 9 Jan 2026 15:19:20 +0100 [thread overview]
Message-ID: <aWEOaFpjj5DhFBlC@pks.im> (raw)
In-Reply-To: <f4gLTILYbAvRqE-aKM3PTyIajeuZBM2Vgo5V66Q8gI6gpI0niPpz8w_lMa29V4Rou2TJ95SKwm2B16KitVrt47KtCzY-eRBm7kemh0iw82s=@protonmail.com>
On Thu, Jan 08, 2026 at 04:52:05PM +0000, Tsahi Elkayam wrote:
> The indexed_table_ref_iter_next() function provides reverse mappings from
> object IDs to references. It currently accesses ref->value.val2 without
> checking the reference's value_type, leading to undefined behavior when
> encountering unpeeled references (REFTABLE_REF_VAL1).
>
> While the current "obj" table implementation is suboptimal—it yields all
> reference records within a block and relies on manual filtering—this
> manual comparison is necessary to ensure the yielded record actually
> matches the target OID prefix requested by the caller.
It's correct to yield all ref records of an indexed ref block, as any of
its refs may point to the object ID. What's incorrect is that we:
- Don't seek to the correct obj index block when creating the
iterator. This means that we'll also seek into ref blocks that won't
even contain any ref with the desired object ID.
- Don't abort iterating over the obj index blocks once we see that its
object IDs no longer match.
So this needs a bit of rephrasing. Please feel free to copy these two
bullet points as-is.
> Fix the undefined behavior by checking the value_type before performing
> the memory comparison. Additionally, replace the "/* BUG */" comment
> with a TODO explaining the current implementation's inefficiency, as
> suggested by the maintainer.
I wouldn't refer to myself as maintainer, I'm very happy to let Junio
have that role :) You can for example simply add a "Helped-by:" trailer
that refers to me.
> diff --git a/reftable/iter.c b/reftable/iter.c
> index 2ecc52b336..2eee65bb1e 100644
> --- a/reftable/iter.c
> +++ b/reftable/iter.c
> @@ -171,12 +171,19 @@ static int indexed_table_ref_iter_next(void *p, struct reftable_record rec)
> }
> continue;
> }
> - /* BUG */
> - if (!memcmp(it->oid.buf, ref->value.val2.target_value,
> - it->oid.len) ||
> - !memcmp(it->oid.buf, ref->value.val2.value, it->oid.len)) {
> +
> + /*
> + * TODO: The current implementation is suboptimal as it yields
> + * all ref records in the block rather than filtering by the
> + * OID prefix. This manual comparison is still necessary.
> + */
And this needs a bit of rephrasing to represent the above. For example:
/*
* TODO: The current implementation is suboptimal as:
*
* - We don't seek to the first obj record that matches our OID
* prefix.
*
* - We don't abort iteration once the OID prefix doesn't match
* anymore.
*
* We don't have any users of this interface in-tree, but once we
* add any we should probably try to fix this interface.
*/
Thanks!
Patrick
prev parent reply other threads:[~2026-01-09 14:19 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-04 10:46 [PATCH v2] reftable/iter: fix undefined behavior in indexed_table_ref_iter_next Tsahi Elkayam
2026-01-05 14:59 ` Patrick Steinhardt
2026-01-08 16:52 ` [PATCH v2] reftable/iter: fix UB " Tsahi Elkayam
2026-01-09 14:19 ` Patrick Steinhardt [this message]
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=aWEOaFpjj5DhFBlC@pks.im \
--to=ps@pks$(echo .)im \
--cc=Tsahi.Elkayam@protonmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/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