Re: dd SIGUSR1 race

[Pádraig Brady]

On 09/26/2014 07:27 AM, Bernhard Voelker wrote:
> On 09/25/2014 03:12 PM, Pádraig Brady wrote:
>> On 09/25/2014 01:26 PM, Federico Simoncelli wrote:
>>> ----- Original Message -----
>>>> From: "Pádraig Brady" <address@hidden>
>>>> To: "Coreutils" <address@hidden>
>>>> Cc: "Federico Simoncelli" <address@hidden>
>>>> Sent: Monday, September 22, 2014 5:25:58 PM
>>>> Subject: dd SIGUSR1 race
>>>>
>>>> There is a race with dd setting up the signal handler for SIGUSR1,
>>>> and some async process sending it.  If dd loses the race it's killed.
> 
> Just for the record: even the little shell example in dd's manpage seems
> to make the program loose the race quite frequently. The following is just
> changed a bit to demonstrate that dd(1) didn't even have the chance to
> create the output file:
> 
>   $ dd if=/dev/zero of=file & pid=$! ; kill -USR1 $pid
>   [1] 21271
>   [1]+  User defined signal 1   dd if=/dev/zero of=file
> 
>   $ test -e file || echo 'dd killed'
>   dd killed
> 
>>>> Though doing the above would still need existing apps to change
>>>> (to ignore SIGUSR1), so instead it might be better to just support
>>>> a simpler mechanism through something like a "status=progress" option,
>>>> which would avoid the general issues and awkwardness of signals as 
>>>> mentioned
>>>> here:
>>>>   
>>>> http://lists.gnu.org/archive/html/bug-coreutils/2004-04/threads.html#00127
>>>>
>>>> I'm leaning towards the latter option here.
> 
>>> - how often should it be reported/updated? (every x seconds or x byte
>>>   transferred? configurable?)
>>
>> We could support status=none,xfer to only output the above transfer 
>> statistics line.
>> I suppose we could do this every second but that would be approximate due
>> to blocking reads() and writes().
> 
> The current implementation has the same minor issue, so I don't see a problem 
> here.
> 
> I only see a problem with removing the existing behavior: if we remove the
> SIGURS1 handler, then dd(1) would be killed if an existing script relies
> on dd(1) to work as before.
> So should we leave the racy stuff in for compatibility? Well, we could enable
> the new status=xfer option in the signal handler, but this would maybe also
> change the behavior the caller would expect.

We'd definitely leave the existing SIGUSR1 handling in place
but we could augment it to allow avoiding races.

Option1:
  Unblock SIGUSR1 explicitly, allowing the parent to mask that and avoid races,

Option2:
  Set the handler even if SIGUSR1 was set to ignored.

Note the default behavior for SIGINFO on BSD is to discard it,
so that doesn't have the "kill dd by mistake" issue, but does it seem
have the possibility to lose SIGINFO, so that for robustness
they'd have to be resent after a timeout.
Now that doesn't seem too onerous since you'd have to be dealing
with timeouts anyway, so one could take the same approach with existing GNU dd
implementations. I.E. just set SIGUSR1 to ignore in the parent
before the fork and then reset.
Another reason to favor Option2 is that can be easily controlled
from the shell using: trap '' USR1
Note we only print stats on receiving the SIGUSR1 so I don't see
much issue in requisitioning it (even though not strictly POSIX compliant).
Also we can even suppress that stats output if problematic with status=none.

Therefore I'm leaning towards option2.
The attached patch supports option2 by fixing up the various races.

Note this doesn't preclude also having an explicit option
to periodically output the statistics, though I'd be slightly
in favor of avoiding that if possible.

cheers,
Pádraig.

dd-siginfo.patch
Description: Text Data