From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 02 Feb 2024 05:21:39 GMT
Subject: Re: [PATCH v2 3/9] reftable/stack: fix parameter validation when compacting range
Message-ID: <Zbx744Uln_UyyyYi@tanuki>
In-Reply-To: <87o7d0p38k.fsf@iotcl.com>

On Thu, Feb 01, 2024 at 05:15:14PM +0100, Toon Claes wrote:
> 
> Patrick Steinhardt <ps@pks.im> 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