On Fri, Aug 02, 2024 at 04:00:52PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
