bug#32375: Bug Gzip v1.9

[Johannes Przybilla]

Hello.

We believe to have found a bug in gzip's signal handler.
Attached will you find our bug report.
Excited to hear from you!

Greetings from Germany,
Johannes Przybilla

[Paul Eggert]

Thanks for reporting the bug. Although I doubt whether it can occur on any
practical platform, we should fix it even if it's just theoretical. I installed
into gzip master on savannah the attached patch, which I hope fixes the problem;
please give it a try.

The SYMBIOSYS tool did not discover a related theoretical bug with the
'caught_signals' variable, which also is used inside a signal handler despite
not being volatile, so you might want to look into that. The attached patch
should fix this related bug too.

I am CC'ing this to Bdale Garbee, since much of this patch brings in a Gnulib
replacement for sigaction that I expect nowadays is used only on mingw, and
Bdale is my mingw guru (see Bug#32305).

[Antonio Diaz Diaz]

Post by Paul Eggert
Thanks for reporting the bug. Although I doubt whether it can occur on
any practical platform, we should fix it even if it's just theoretical.

I have applied to lzip a change similar to yours, but I'm not sure it
fixes the theoretical problem.

Post by Paul Eggert
+static char volatile remove_ofname[MAX_PATH_LEN];

I see two problems with this fix. The first is that it limits filename
size. It seems that on GNU/Hurd systems there is no limit to the size of
a file name, so this would place an artificial limit to the use of lzip
on such systems.

The second is that posix states the following:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03
------------------------------------------------------------------------
If the process is multi-threaded, or if the process is single-threaded
and a signal handler is executed other than as the result of:
* The process calling abort(), raise(), kill(), pthread_kill(), or
sigqueue() to generate a signal that is not blocked
* A pending signal being unblocked and being delivered before the
call that unblocked it returns

the behavior is undefined if the signal handler refers to any object
other than errno with static storage duration other than by assigning a
value to an object declared as volatile sig_atomic_t, or if the signal
handler calls any function defined in this standard other than one of
the functions listed in the following table.
------------------------------------------------------------------------

The last paragraph seems to imply that a signal handler can't read any
static object, only write to 'volatile sig_atomic_t'. This is how I
implemented the Ctrl-C handler in ddrescue, but in the case of gzip and
lzip, polling a sig_atomic_t variable may be inefficient or cause a
noticeable delay. I find posix too restrictive in this respect.

Any thoughts?


Antonio.

[Paul Eggert]

Post by Antonio Diaz Diaz
I find posix too restrictive in this respect.

Yes, me too.

The file name length limit has been in gzip since forever, and is a
separate issue (e.g. it happens regardless of signals). So if it is to
be fixed it should be a separate bug number.

[Johannes Przybilla]

I agree that the POSIX standard is quite restrictive in this respect.
However I believe that is for a good reason.
Performing non-atomic memory operations on static objects in a signal handler can cause problems with reentrancy.
This can lead to undefined behaviour for example in the case of nested signal handler calls that update the same static object.

Post by Paul Eggert

Post by Antonio Diaz Diaz
I find posix too restrictive in this respect.

Yes, me too.
The file name length limit has been in gzip since forever, and is a
separate issue (e.g. it happens regardless of signals). So if it is to
be fixed it should be a separate bug number.

[Paul Eggert]

Post by Johannes Przybilla
This can lead to undefined behaviour for example in the case of nested signal handler calls

These can't happen in gzip, so we should be OK here.

[Johannes Przybilla]

Hello Paul.

I only recently got around to check the fix you committed.
It seems to me that after 'volatile_strcpy (remove_ofname, ofname);‘ the 'remove_ofname‘ variable is not used anymore.
The 'xunlink' function in 'remove_output_file' is still called on 'ofname' instead of 'remove_ofname‘.
Therefore the non-volatile access still occurs in the signal handler.
Could you look into that?

Best regards,
Johannes

Post by Paul Eggert

Post by Johannes Przybilla
This can lead to undefined behaviour for example in the case of nested signal handler calls

These can't happen in gzip, so we should be OK here.

[Paul Eggert]

Post by Johannes Przybilla
Therefore the non-volatile access still occurs in the signal handler.

Good catch, thanks. I installed the attached further patch; please give it a
try. Signal handlers are such a pain.

[Antonio Diaz Diaz]

Post by Johannes Przybilla
I agree that the POSIX standard is quite restrictive in this
respect. However I believe that is for a good reason. Performing
non-atomic memory operations on static objects in a signal handler
can cause problems with reentrancy. This can lead to undefined
behaviour for example in the case of nested signal handler calls that
update the same static object.

This is not at all the case in gzip and gzip-like compressors. In them
the handler is executed just once to perform some cleanup operations
(mainly remove the partial output file), an then exit. The handler does
not perform any non-atomic memory operations on static objects, and
control never returns to the main thread. IMO posix should allow to this
kind of cleanup handlers read access to any static object.

In fact, as Paul pointed out, any practical platform probably allows
such read access because many of the functions posix allows to be called
from a handler require a filename, which in the case of a cleanup
handler is most probably a static object.