Re: [PATCH] reftable/iter: fix undefined behavior in indexed_table_ref_iter_next

From: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
Date: Jan 3, 2026, 07:35 UTC
Message-ID: <CALE2CrQTvHeu21yLXtRg=A6ak9AB_vvwPirQNFDjZ2AmhoTzTQ@mail.gmail.com>
In-Reply-To: <Q0zfHYp-_TO2h_5PXPG9KjHwpMKIf2o2u2dsaoAjIsScmA3W6t7IvqIEeLfM7auEFIQyazlNnA3MGAuS4AANF0yfEBJAjkU1bWp-NH9m89U=@protonmail.com>

Hi Tsahi,

Thanks for working on this.

The issue and fix make sense to me. Guarding access to the val2 members behind a value_type check avoids the undefined behavior noted by the existing comment, and explicitly handling REFTABLE_REF_VAL1 here matches the pattern already used in filtering_ref_iterator_next().

I didn’t spot any issues with the control flow or logic in this change.

Thanks for addressing this.

Pushkar

On Sat, Jan 3, 2026 at 12:47 AM Tsahi Elkayam <Tsahi.Elkayam@protonmail.com> wrote:

Show 50 quoted lines

>
>
>
>   The indexed_table_ref_iter_next() function accesses ref->value.val2
>   without first checking the ref's value_type. This is undefined behavior
>   when the ref is not of type REFTABLE_REF_VAL2.
>
>   The correct pattern is already used in filtering_ref_iterator_next()
>   which checks value_type before accessing the appropriate union member.
>   Apply the same pattern here:
>
>    - Check for REFTABLE_REF_VAL2 before accessing val2 members
>    - Add missing check for REFTABLE_REF_VAL1 to handle single-value refs
>
>   This was marked with a "/* BUG */" comment indicating the issue was
>   known but not yet fixed.
>
>   Signed-off-by: Tsahi Elkayam <Tsahi.Elkayam@protonmail.com>
>   ---
>    reftable/iter.c | 13 ++++++++-----
>    1 file changed, 8 insertions(+), 5 deletions(-)
>
>   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,15 @@ 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)) {
>   +             if (ref->value_type == REFTABLE_REF_VAL2 &&
>   +                 (!memcmp(it->oid.buf, ref->value.val2.target_value,
>   +                          it->oid.len) ||
>   +                  !memcmp(it->oid.buf, ref->value.val2.value, it->oid.len)))
>   +                     return 0;
>   +
>   +             if (ref->value_type == REFTABLE_REF_VAL1 &&
>   +                 !memcmp(it->oid.buf, ref->value.val1, it->oid.len))
>                         return 0;
>   -             }
>         }
>    }
>
>   --
>   2.37.1 (Apple Git-137.1)
>