Re: [PATCH] config: use a static lock_file struct

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Aug 30, 2017, 03:25 UTC
Message-ID: <01375356-5d39-99af-9e91-35083ed03f42@alum.mit.edu>
In-Reply-To: <20170829191217.dt65wazf7qh5qs3k@sigill.intra.peff.net>

On 08/29/2017 09:12 PM, Jeff King wrote:

Show 35 quoted lines

> On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:
> 
>>> -- >8 --
>>> Subject: [PATCH] config: use a static lock_file struct
>>>
>>> When modifying git config, we xcalloc() a struct lock_file
>>> but never free it. This is necessary because the tempfile
>>> code (upon which the locking code is built) requires that
>>> the resulting struct remain valid through the life of the
>>> program. However, it also confuses leak-checkers like
>>> valgrind because only the inner "struct tempfile" is still
>>> reachable; no pointer to the outer lock_file is kept.
>>
>> Is this just due to a limitation in the tempfile code?  Would it be
>> possible to improve the tempfile code such that we don't need to require
>> that a tempfile, once created, is required to exist for the remaining
>> life of the program?
> 
> Yes. Like I wrote below:
> 
>>> ---
>>> In the long run we may want to drop the "tempfiles must remain forever"
>>> rule. This is certainly not the first time it has caused confusion or
>>> leaks. And I don't think it's a fundamental issue, just the way the code
>>> is written. But in the interim, this fix is probably worth doing.
> 
> The main issue is that the caller needs to make sure they're removed
> from the list (via commit or rollback) before being freed.
> 
> As far as I know anyway. This restriction dates back to the very early
> days of the lockfile code and has been carried through the various
> tempfile-cleanup refactorings over the years (mostly because each was
> afraid to make functional changes).
> 
> +cc Michael, who did most comprehensive cleanup of that code.

It was surprisingly hard trying to get that code to do the right thing, non-racily, in every error path. Since `remove_tempfiles()` can be called any time (even from a signal handler), the linked list of `tempfile` objects has to be kept valid at all times but can't use mutexes. I didn't have the energy to keep going and make the lock objects freeable.

I suppose the task could be made a bit easier by using `sigprocmask(2)` or `pthread_sigmask(3)` to make its running environment a little bit less hostile.

Michael