Re: [PATCH 4/8] reftable/stack: simplify tracking of table locks

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 5, 2024, 12:11 UTC
Message-ID: <ZrDBet_24S4J8vhN@tanuki>
In-Reply-To: <xmqqy15e8r8b.fsf@gitster.g>

On Fri, Aug 02, 2024 at 04:00:52PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > @@ -1192,8 +1192,8 @@ static int stack_compact_range(struct reftable_stack *st,
> >  
> >  done:
> >  	rollback_lock_file(&tables_list_lock);
> > -	for (i = first; table_locks && i <= last; i++)
> > -		rollback_lock_file(&table_locks[i - first]);
> > +	for (i = 0; table_locks && i < nlocks; i++)
> > +		rollback_lock_file(&table_locks[i]);
> 
> This is a true bugfix, isn't it?  If we failed to create lock file
> somewhere in the middle, we used to still go ahead and attempted
> rollback_lock_file() on all of them.  Now we rollback only what we
> successfully called hold_lock_file_for_update().
> 
> I wonder why nobody segfaulted where after a failed lock.  The
> answer probably is that lk->tempfile that is NULL will safely bypass
> most of the things because is_tempfile_active() would say "false" on
> such a lockfile.  But still it probably still were wrong to call
> rollback_lock_file() on a "struct lockfile" full of NUL-bytes, and
> it is good that we no longer do that.

I don't think it is. `table_locks` is an array of `struct lockfile` and is allocated with calloc(3P), so we know that each uninitialized lock will be all zeroes. For each uninitialized lock, `rollback_lock_file()` calls `delete_tempfile(&lk->tempfile)`, which derefs the pointer and thus essentially calls `!is_tempfile_active(lk->tempfile)`, and that ultimately ends up checking whethere `tempfile` is a `NULL` pointer or not. And as the structs were zero-initialized, it is a `NULL` pointer and thus we bail out.

We also have tests that exercise this logic, so it also seems to be fine in practice.

Patrick