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

[Michael Haggerty <mhagger@alum.mit.edu>]

On Wed, Aug 30, 2017 at 6:31 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote:
>> 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.
>
> I think there are really two levels of carefulness here:
>
>   1. Avoiding complicated things during a signal handler that may rely
>      on having a sane state from the rest of the program (e.g.,
>      half-formed entries, stdio locks, etc).
>
>   2. Being truly race-free in the face of a signal arriving while we're
>      running arbitrary code that might have a tempfile struct in a funny
>      state.
>
> I feel like right now we meet (1) and not (2). But I think if we keep to
> that lower bar of (1), it might not be that bad. We're assuming now that
> there's no race on the tempfile->active flag, for instance. We could
> probably make a similar assumption about putting items onto or taking
> them off of a linked list (it's not really atomic, but a single pointer
> assignment is probably "atomic enough" for our purposes).
>
> Or I dunno. There's a lot of "volatile" modifiers sprinkled around.
> Maybe those are enough to give us (2) as well (though in that case, I
> think we'd still be as OK with the list manipulation as we are with the
> active flag manipulation).

I did in fact strive for both (1) and (2), though I know that there
are a couple of short races remaining in the code (plus who knows how
many bugs).

Actually, I think you're right that a single "probably-atomic" pointer
assignment would be enough to remove an entry from the list safely. I
was thinking about what it would take to make the list thread-safe
(which it currently is not, but probably doesn't need to be(?)).
Modifying the linked list is not that difficult if you assume that
there is only a single thread modifying the list at any given time.

Michael