ADBM-2801: Disable logging in signal handlers

[silent-observer]

Disable logging in signal handlers

Currently pgbackrest allows logging in signal handler exitOnSignal in exit.c.
Logging uses various locking operations (like gmtime) and also static internal
state, so if the signal occurs during logging, and then the signal handler
tries to log something else, this can lead to deadlocks and undefined
behavior.

Instead disable all logging inside signal handlers, since it does not contain
crucial info anyway. Tests are not added since there doesn't seem to be a
simple way to check the logs of a process that already exited.

[ShapatinAA]

Why not to add blocking of signals in signal handler?

[silent-observer]

Why not to add blocking of signals in signal handler?

They're already blocked. The issue occurs when logging is happening somewhere else, and then a signal occurs, trying to do logging again. We don't need signals within signals to achieve this.

[hilltracer]

The issue occurs when logging is happening somewhere else, and then a signal occurs, trying to do logging again. We don't need signals within signals to achieve this.

Do I understand right, that removing the FUNCTION_LOG_ lines is enough to fix the deadlock?
Do we need to disable logging globally inside the signal handler?

Looks like it's unsafe in not release builds. Because it contains
FUNCTION_TEST... macros and ASSERTs.

Also it read and writes multiple non-atomic global variables.

[hilltracer]

Oh, I see the problem will be here in this case:

pgbackrest/src/command/exit.c

Lines 109 to 113 in 3b68e3c

	FUNCTION_LOG_BEGIN(logLevelDebug);
	FUNCTION_LOG_PARAM(INT, result);
	FUNCTION_LOG_PARAM(BOOL, error);
	FUNCTION_LOG_PARAM(ENUM, signalType);
	FUNCTION_LOG_END();

[silent-observer]

Do I understand right, that removing the FUNCTION_LOG_ lines is enough to fix the deadlock?
Do we need to disable logging globally inside the signal handler?

No, it's not enough, there's more logging happening in exitSafe. It is used in other cases, so we can't just remove logging from there too, hence the global disabling.

Looks like it's unsafe in not release builds. Because it contains
FUNCTION_TEST... macros and ASSERTs.

We can guarantee that the asserts will never fire in this case (all the parameters are hardcoded after all), and FUNCTION_TEST macros don't actually log anything, they simply save parameters into internal stacktrace. Also, this bug is rare and only really occurs if pgbackrest stop is executed many times in a row, which current tests never do.

Also it read and writes multiple non-atomic global variables.

We do not have concurrent access here, so it's fine. The signal handler interrupts the main thread of execution, so there is no threat of data races. The only issue is that interrupts can happen while the thread is holding a lock or while the logging variables are in an inconsistent state.