On Thu, Feb 01, 2024 at 05:15:14PM +0100, Toon Claes wrote:
>
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index d084823a92..b6b24c90bf 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -1146,12 +1146,14 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
> > done:
> > free_names(delete_on_success);
> >
> > - listp = subtable_locks;
> > - while (*listp) {
> > - unlink(*listp);
> > - listp++;
> > + if (subtable_locks) {
> > + listp = subtable_locks;
> > + while (*listp) {
> > + unlink(*listp);
> > + listp++;
> > + }
> > + free_names(subtable_locks);
> > }
> > - free_names(subtable_locks);
> > if (lock_file_fd >= 0) {
> > close(lock_file_fd);
> > lock_file_fd = -1;
>
> Technically, this change is not needed, because `free_names()` deals
> with NULL pointers already.
It actually is, but it's easy to miss. The reason why I've started to
guard the code isn't `free_names()`. It's rather the `while (*listp)`,
where we dereference `listp` unconditionally. Before these refactorings,
`subtable_locks` would never be `NULL`. Now it can be though, and in
that case we would be dereferencing a `NULL` pointer.
Patrick
Attachment:
signature.asc
Description: PGP signature
