On 08/29/2017 09:12 PM, Jeff King wrote: > 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
