[pve-devel] do not falsely overwrite global signal handlers
Thomas Lamprecht
t.lamprecht at proxmox.com
Wed Sep 6 13:29:02 CEST 2017
Yesterday I found out that my pvestatd did not correctly shutdown since
quite a bit, my physical host says that the date where this pattern
started is around July the 4th. After a journey through a few rabbit
holes I saw that this problem comes from pve-storage's commit
e53050ed13bbe9c8e8122c0b7b34b2e4b33f242e (Fixes: #1415 do not block
pvestatd when nfsd is stopped on the server side)
Which introduces the usage of a also new helper 'run_fork_with_timeout',
this helper came in with the same series in pve-common as commit
72fba9114b48b9567ac5d22c9739fab6eb83c988 (Add run_fork_with_timeout
utility). It executes code in a forked child and returns the result it
got over a pipe. To avoid leaving behind zombies it traps a few signals
which could terminate the parent process, to avoid overwriting signal
handler from the call chain above it uses perls 'local' keyword, making
the change local to the current sub-method only. But, the local keyword
got used wrong:
local $SIG{INT} = $SIG{TERM} = ... = $SIG{PIPE} = sub { ... };
The assumption was that all of those signal handler were only
overwritten locally.
But the local perl keyword only holds for the first variable, $SIG{INT}.
Either adding a 'local' in front of each variable or putting all
variables in a list with local in front would address this, e.g.:
local $SIG{INT} = local $SIG{TERM} = local ... = sub { ... };
This is probably the result of a copy paste error, as I found this wrong
pattern used on a few other places too.
Now, the problem with pvestatd was that it does not runs in worker and
thus the usage of 'run_fork_with_timeout' overwrote not "just" a childs
signal handler but the ones from the actual daemon itself, which where
carefully setup by PVE::Daemon.
This series fixes all occurrences of the wrong pattern I found.
On some places it may not be directly harmful as it was code run in a
short living worker. Fix them none-the less as a) its still wrong and b)
can result in someone just copying this over to a place were it really
matters.
More information about the pve-devel
mailing list