Re: dd SIGUSR1 race

[Bernhard Voelker]

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.

Have a nice day,
Berny