On Wed, Aug 30, 2017 at 6:31 AM, Jeff King <peff@xxxxxxxx> 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
