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

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

On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote:

quoted

quoted

-- >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:

quoted

quoted

---
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