tests: ASSERT_SIGNAL: Do not allow parent to hallucinate it is the child
assert_signal_internal() returns 0 in two distinct cases: 1. In the child process (immediately after fork returns 0). 2. In the parent process, if the child exited normally (no signal). ASSERT_SIGNAL fails to distinguish these cases. When a child exited normally (case 2), the parent process receives 0, incorrectly interprets it as meaning it is the child, and re-executes the test expression inside the parent process. Goodness gracious! This causes two severe test integrity issues: 1. False positives. The parent can run the expression, succeed, and call _exit(EXIT_SUCCESS), causing the test to pass even though no signal was raised. 2. Silent truncation. The _exit() call in the parent terminates the test runner prematurely, preventing subsequent tests in the same file from running. Example of the bug in action, from #39674: ASSERT_SIGNAL(fd_is_writable(closed_fd), SIGABRT) This test should fail (fd_is_writable does not SIGABRT here), but with the bug, the parent hallucinated being the child, re-ran the expression successfully, and exited with success. Fix this by refactoring assert_signal_internal() to be much more strict about separating control flow from data. The signal status is now returned via a strictly typed output parameter, guaranteeing that determining whether we are the child is never conflated with whether the child exited cleanly.
This commit is contained in:
@@ -166,4 +166,50 @@ TEST(ASSERT_SIGNAL_exit_code_vs_signal) {
|
||||
ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(0), SIGABRT), SIGABRT);
|
||||
}
|
||||
|
||||
/* Regression test for issue where returning 0 from assert_signal_internal() was ambiguous.
|
||||
*
|
||||
* In the bug case, when assert_signal_internal() returned 0, it could mean two different things:
|
||||
*
|
||||
* 1. We're in the child process (fork() just returned 0)
|
||||
* 2. We're in the parent and the child exited normally (no signal)
|
||||
*
|
||||
* The ASSERT_SIGNAL macro couldn't distinguish between these cases. When case #2 occurred, the macro would
|
||||
* re-enter the "if (_r == 0)" block, re-run the expression in the parent, and call _exit(EXIT_SUCCESS),
|
||||
* causing tests to incorrectly pass even when no signal occurred.
|
||||
*
|
||||
* The fix separates the question of which process we are in from which signal occurred:
|
||||
*
|
||||
* - assert_signal_internal() now returns ASSERT_SIGNAL_FORK_CHILD (0) or ASSERT_SIGNAL_FORK_PARENT (1) to
|
||||
* indicate execution path
|
||||
* - The actual signal/status is passed via an output parameter (*ret_status)
|
||||
*
|
||||
* This allows the macro to unambiguously distinguish between being the child (path ==
|
||||
* ASSERT_SIGNAL_FORK_CHILD) and being the parent when the child has exited normally (path ==
|
||||
* ASSERT_SIGNAL_FORK_PARENT && status == 0).
|
||||
*
|
||||
* This test verifies that when a child exits normally (with exit code 0), ASSERT_SIGNAL correctly detects
|
||||
* that NO signal was raised, rather than being confused and thinking it's still in the child process.
|
||||
*/
|
||||
TEST(ASSERT_SIGNAL_exit_vs_child_process) {
|
||||
/* When a child calls _exit(0), it exits normally with code 0 (no signal). The parent's
|
||||
* assert_signal_internal() returns ASSERT_SIGNAL_FORK_PARENT, and sets ret_status to 0, meaning
|
||||
* there was no signal. This should NOT be confused with being the child process. The inner
|
||||
* ASSERT_SIGNAL expects SIGABRT but sees no signal, so it should fail, which the outerj
|
||||
* ASSERT_SIGNAL catches. */
|
||||
ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(EXIT_SUCCESS), SIGABRT), SIGABRT);
|
||||
}
|
||||
|
||||
TEST(ASSERT_SIGNAL_basic) {
|
||||
/* Correct behavior: expression raises expected signal */
|
||||
ASSERT_SIGNAL(abort(), SIGABRT);
|
||||
ASSERT_SIGNAL(raise(SIGTERM), SIGTERM);
|
||||
ASSERT_SIGNAL(raise(SIGSEGV), SIGSEGV);
|
||||
ASSERT_SIGNAL(raise(SIGILL), SIGILL);
|
||||
|
||||
/* Wrong signal: inner ASSERT_SIGNAL expects SIGABRT but gets SIGTERM, so it fails (aborts), which
|
||||
* outer ASSERT_SIGNAL catches. */
|
||||
ASSERT_SIGNAL(ASSERT_SIGNAL(raise(SIGTERM), SIGABRT), SIGABRT);
|
||||
ASSERT_SIGNAL(ASSERT_SIGNAL(raise(SIGKILL), SIGTERM), SIGABRT);
|
||||
}
|
||||
|
||||
DEFINE_TEST_MAIN(LOG_INFO);
|
||||
|
||||
Reference in New Issue
Block a user