Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals

> On Fri, Jan 07, 2022 at 11:55:47AM +0100, Patrick Steinhardt wrote:
>> diff --git a/transport.c b/transport.c
>> index 92ab9a3fa6..2a3e324154 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1456,13 +1456,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
>>   	return rc;
>>   }
>>
>> -void transport_unlock_pack(struct transport *transport)
>> +void transport_unlock_pack(struct transport *transport, unsigned int flags)
>>   {
>> +	int in_signal_handler = !!(flags & TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER);
>>   	int i;
>>
>>   	for (i = 0; i < transport->pack_lockfiles.nr; i++)
>> -		unlink_or_warn(transport->pack_lockfiles.items[i].string);
>> -	string_list_clear(&transport->pack_lockfiles, 0);
>> +		if (in_signal_handler)
>> +			unlink(transport->pack_lockfiles.items[i].string);
>> +		else
>> +			unlink_or_warn(transport->pack_lockfiles.items[i].string);
> 
> This puzzled me when I first read it. But unlink_or_warn() isn't
> reentrant because it performs buffered IO on stderr, so if we reached
> this signal handler while executing another function call modifying
> those same buffers, the call within the signal handler would have
> undefined behavior.
> 
> So that makes sense: freeing (with string_list_clear() below) and
> performing buffered IO (with unlink_or_warn() here as just described)
> are certainly off the table.
> 
> But is unlink() safe as-is? I'm not so sure. Reading signal-safety(7),
> it's clearly on the list of functions that are OK to call. But reading
> the errno section:
> 
>      errno
>        Fetching and setting the value of errno is async-signal-safe
>        provided that the signal handler saves errno on entry and restores
>        its value before returning.
> 
> We certainly not doing that, though that's nothing new, and so I wonder
> why it doesn't seem to be an issue in practice.