dd(1): neutralize SIGINT while non-async-signal safe code is executing

[kib]

Summary

making the SIGINT handler (the terminate() function) safe to execute at
any interruption moment.  This fixes a race in
5807f35c541c26bbd91a3ae12506cd8dd8f20688 where SIGINT delivered right
after the check_terminate() but before a blocking syscall would not
cause abort.

Do it by setting the in_io flag around potentially blocking io syscalls.
If handler sees the flag, it terminates the program.  Otherwise,
termination is delegated to the before_io/after_io fences.

[des]

You are now blocking SIGINT whenever dd is not performing primary I/O. I don't think that's right.

[kib]

In D40281#918424, @des wrote:

You are now blocking SIGINT whenever dd is not performing primary I/O. I don't think that's right.

How the effects of that are different than the current code, where the ^C handler just set a flag and returns? If my code is not right there, then the current unmodified code is not right either. I believe the correctness relies on the fact that dd performs a cpu-bound processing between i/o syscalls.

[andrew_tao173.riddles.org.uk]

In D40281#918468, @kib wrote:

How the effects of that are different than the current code, where the ^C handler just set a flag and returns?

The one difference of any significance would be if the program is blocked on writing a progress output to stderr (e.g. if that has been directed to a pipe) when you ^C it; a handler installed without SA_RESTART that just sets a flag could nevertheless cause an EINTR from the write, whereas with signals blocked the program will remain blocked.

I don't know if anyone cares about this.

[kib]

In D40281#918781, @andrew_tao173.riddles.org.uk wrote:

In D40281#918468, @kib wrote:

How the effects of that are different than the current code, where the ^C handler just set a flag and returns?

The one difference of any significance would be if the program is blocked on writing a progress output to stderr (e.g. if that has been directed to a pipe) when you ^C it; a handler installed without SA_RESTART that just sets a flag could nevertheless cause an EINTR from the write, whereas with signals blocked the program will remain blocked.

I don't know if anyone cares about this.

Indeed, I can fix this case as well, but IMO it is overkill. It should work by wrapping err()/warn*()/fprintf(stderr,...) with code that sets SIGINT handler to something that records the signal, and then unblocks it. It is esp. worthless because errors from stderr output are not handled.

[kib]

I think I realized how this might be fixed more elegant, without compromising even the EINTR on stderr behavior. Instead of sys-blocking the signal, I can do it by exchanging two flags between the handler and fences. Updated patch follows.

[kib]

Do not block SIGINT, use flags instead. This slightly resembles Peterson lock.

[des]

This is a massively over-engineered solution to a problem that doesn't need solving. I'm not sure why you're pursuing this. The problem I set out to solve has been solved and the remaining race is insignificant.

[andrew_tao173.riddles.org.uk]

bin/dd/misc.c

168

Race here: if the signal delivers after the read of sigint_seen but before the write to in_io, the signal is not processed until after the I/O. This is basically no better than the plain flag version.

On the other hand, if you wrote in_io first, then a barrier, then check sigint_seen, then either the signal is delivered before the in_io write and so sigint_seen is set true, or it's delivered after and the remaining code is not reached because the handler exits.

[kib]

In D40281#919091, @des wrote:

This is a massively over-engineered solution to a problem that doesn't need solving. I'm not sure why you're pursuing this. The problem I set out to solve has been solved and the remaining race is insignificant.

What is over-engineered? The current solution is quite simple and solves the race. The race is the bug, whatever subtle it is, and I do care about it.

bin/dd/misc.c

168

You are right of course, thanks.

[kib]

Fix the order in before_io()