From 408e8d361fc179dd43f3ba91b9691abc52903134 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Wed, 19 Nov 2025 16:49:22 +0800 Subject: [PATCH 1/4] tests: Avoid variable shadowing in ASSERT_SIGNAL The ASSERT_SIGNAL macro uses a fixed variable name, `_r`. This prevents nesting the macro (like ASSERT_SIGNAL(ASSERT_SIGNAL(...))), as the inner instance would shadow the outer instance's variable. Switch to using the UNIQ_T helper to generate unique variable names at each expansion level. This allows the macro to be used recursively, which is required for upcoming regression tests regarding signal handling logic. --- src/shared/tests.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/shared/tests.h b/src/shared/tests.h index 4d546c6ab9..1605778fb6 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -597,18 +597,19 @@ int assert_signal_internal(void); #ifdef __COVERITY__ # define ASSERT_SIGNAL(expr, signal) __coverity_check__(((expr), false)) #else -# define ASSERT_SIGNAL(expr, signal) \ +# define ASSERT_SIGNAL(expr, signal) __ASSERT_SIGNAL(UNIQ, expr, signal) +# define __ASSERT_SIGNAL(uniq, expr, signal) \ ({ \ ASSERT_TRUE(SIGNAL_VALID(signal)); \ - int _r = assert_signal_internal(); \ - ASSERT_OK_ERRNO(_r); \ - if (_r == 0) { \ + int UNIQ_T(_r, uniq) = assert_signal_internal(); \ + ASSERT_OK_ERRNO(UNIQ_T(_r, uniq)); \ + if (UNIQ_T(_r, uniq) == 0) { \ expr; \ _exit(EXIT_SUCCESS); \ } \ - if (_r != signal) \ + if (UNIQ_T(_r, uniq) != signal) \ log_test_failed("\"%s\" died with signal %s, but %s was expected", \ - #expr, signal_to_string(_r), signal_to_string(signal)); \ + #expr, signal_to_string(UNIQ_T(_r, uniq)), signal_to_string(signal)); \ }) #endif From 39adecfcd84250f7e382c220ec7bcb2b0faa5193 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Wed, 19 Nov 2025 16:50:38 +0800 Subject: [PATCH 2/4] tests: ASSERT_SIGNAL: Stop exit codes from masquerading as signals When a child process exits normally (si_code == CLD_EXITED), siginfo.si_status contains the exit code. When it is killed by a signal (si_code == CLD_KILLED or CLD_DUMPED), si_status contains the signal number. However, assert_signal_internal() returns si_status blindly. This causes exit codes to be misinterpreted as signal numbers. This allows failing tests to silently pass if their exit code numerically coincides with the expected signal. For example, a test expecting SIGABRT (6) would incorrectly pass if the child simply exited with status 6 instead of being killed by a signal. Fix this by checking si_code. Only return si_status as a signal number if the child was actually killed by a signal (CLD_KILLED or CLD_DUMPED). If the child exited normally (CLD_EXITED), return 0 to indicate that no signal occurred. --- src/shared/tests.c | 10 +++++++++- src/test/test-tests.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/shared/tests.c b/src/shared/tests.c index 53dda342a4..9dc0980025 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -446,7 +446,15 @@ int assert_signal_internal(void) { if (r < 0) return r; - return siginfo.si_status; + /* si_status means different things depending on si_code: + * - CLD_EXITED: si_status is the exit code + * - CLD_KILLED/CLD_DUMPED: si_status is the signal number that killed the process + * We need to return the signal number only if the child was killed by a signal. */ + if (IN_SET(siginfo.si_code, CLD_KILLED, CLD_DUMPED)) + return siginfo.si_status; + + /* Child exited normally, return 0 to indicate no signal was received, regardless of actual exit */ + return 0; } diff --git a/src/test/test-tests.c b/src/test/test-tests.c index 40112a6762..15e36444d3 100644 --- a/src/test/test-tests.c +++ b/src/test/test-tests.c @@ -137,4 +137,33 @@ TEST(ASSERT_OK_OR) { ASSERT_SIGNAL(ASSERT_OK_OR(-1, -2), SIGABRT); } +/* Regression test for issue where assert_signal_internal() wasn't checking si_code before returning + * si_status. + * + * In the bug case, siginfo.si_status has different meanings depending on siginfo.si_code: + * + * - If si_code == CLD_EXITED: si_status is the exit code (0-255) + * - If si_code == CLD_KILLED/CLD_DUMPED: si_status is the signal number + * + * In the bug case where st_code is not checked, exit codes would be confused with signal numbers. For + * example, if a child exits with code 6, it would incorrectly look like SIGABRT. + * + * This test verifies that exit codes are NOT confused with signal numbers, even when the exit code + * numerically matches a signal number. + */ +TEST(ASSERT_SIGNAL_exit_code_vs_signal) { + /* These exit codes numerically match common signal numbers, but ASSERT_SIGNAL should correctly + * identify them as exit codes (si_code==CLD_EXITED), not signals. The inner ASSERT_SIGNAL expects a + * signal but gets an exit code, so it should fail (aborting with SIGABRT), which the outer + * ASSERT_SIGNAL then catches. */ + + ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(6), SIGABRT), SIGABRT); /* 6 = SIGABRT */ + ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(9), SIGKILL), SIGABRT); /* 9 = SIGKILL */ + ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(11), SIGSEGV), SIGABRT); /* 11 = SIGSEGV */ + ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(15), SIGTERM), SIGABRT); /* 15 = SIGTERM */ + + /* _exit(0) should not be confused with any signal */ + ASSERT_SIGNAL(ASSERT_SIGNAL(_exit(0), SIGABRT), SIGABRT); +} + DEFINE_TEST_MAIN(LOG_INFO); From d759ed527c7c75cffcc9b72dd7b3fcf0854bec2f Mon Sep 17 00:00:00 2001 From: Chris Down Date: Wed, 19 Nov 2025 21:45:40 +0800 Subject: [PATCH 3/4] tests: ASSERT_SIGNAL: Ensure sanitisers do not mask expected signals ASAN installs signal handlers to catch crashes like SIGSEGV or SIGILL. When these signals are raised, ASAN traps them, prints an error report, and then typically terminates the process with a different signal (often SIGABRT) or a non-zero exit code. This interferes with ASSERT_SIGNAL when checking for specific crash signals (for example, checking that a function raises SIGSEGV). In such a case, the test harness sees the ASAN termination signal rather than the expected signal, causing the test to fail. Fix this by resetting the signal handler to SIG_DFL in the child process immediately before executing the test expression. This ensures the kernel kills the process directly with the expected signal, bypassing ASAN's interceptors. --- src/shared/tests.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/shared/tests.h b/src/shared/tests.h index 1605778fb6..2aced04265 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -598,18 +598,19 @@ int assert_signal_internal(void); # define ASSERT_SIGNAL(expr, signal) __coverity_check__(((expr), false)) #else # define ASSERT_SIGNAL(expr, signal) __ASSERT_SIGNAL(UNIQ, expr, signal) -# define __ASSERT_SIGNAL(uniq, expr, signal) \ +# define __ASSERT_SIGNAL(uniq, expr, sgnl) \ ({ \ - ASSERT_TRUE(SIGNAL_VALID(signal)); \ + ASSERT_TRUE(SIGNAL_VALID(sgnl)); \ int UNIQ_T(_r, uniq) = assert_signal_internal(); \ ASSERT_OK_ERRNO(UNIQ_T(_r, uniq)); \ if (UNIQ_T(_r, uniq) == 0) { \ + (void) signal(sgnl, SIG_DFL); \ expr; \ _exit(EXIT_SUCCESS); \ } \ - if (UNIQ_T(_r, uniq) != signal) \ + if (UNIQ_T(_r, uniq) != sgnl) \ log_test_failed("\"%s\" died with signal %s, but %s was expected", \ - #expr, signal_to_string(UNIQ_T(_r, uniq)), signal_to_string(signal)); \ + #expr, signal_to_string(UNIQ_T(_r, uniq)), signal_to_string(sgnl)); \ }) #endif From e21a431ec45ef11f1dffddef0d16fa4fcaece535 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Wed, 19 Nov 2025 22:06:03 +0800 Subject: [PATCH 4/4] 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. --- src/shared/tests.c | 18 ++++++++++++----- src/shared/tests.h | 20 +++++++++++++------ src/test/test-tests.c | 46 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/shared/tests.c b/src/shared/tests.c index 9dc0980025..442fe83045 100644 --- a/src/shared/tests.c +++ b/src/shared/tests.c @@ -425,10 +425,17 @@ void test_prepare(int argc, char *argv[], int log_level) { test_setup_logging(log_level); } -int assert_signal_internal(void) { +/* Returns: + * ASSERT_SIGNAL_FORK_CHILD = We are in the child process + * ASSERT_SIGNAL_FORK_PARENT = We are in the parent process (signal/status stored in *ret_signal) + * <0 = Error (negative errno) + */ +int assert_signal_internal(int *ret_signal) { siginfo_t siginfo = {}; int r; + assert(ret_signal); + r = fork(); if (r < 0) return -errno; @@ -439,7 +446,7 @@ int assert_signal_internal(void) { /* But still set an rlimit just in case */ (void) setrlimit(RLIMIT_CORE, &RLIMIT_MAKE_CONST(0)); - return 0; + return ASSERT_SIGNAL_FORK_CHILD; } r = wait_for_terminate(r, &siginfo); @@ -451,10 +458,11 @@ int assert_signal_internal(void) { * - CLD_KILLED/CLD_DUMPED: si_status is the signal number that killed the process * We need to return the signal number only if the child was killed by a signal. */ if (IN_SET(siginfo.si_code, CLD_KILLED, CLD_DUMPED)) - return siginfo.si_status; + *ret_signal = siginfo.si_status; + else + *ret_signal = 0; - /* Child exited normally, return 0 to indicate no signal was received, regardless of actual exit */ - return 0; + return ASSERT_SIGNAL_FORK_PARENT; } diff --git a/src/shared/tests.h b/src/shared/tests.h index 2aced04265..31b722829d 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -592,7 +592,12 @@ _noreturn_ void log_test_failed_internal(const char *file, int line, const char }) #endif -int assert_signal_internal(void); +enum { + ASSERT_SIGNAL_FORK_CHILD = 0, /* We are in the child process */ + ASSERT_SIGNAL_FORK_PARENT = 1, /* We are in the parent process */ +}; + +int assert_signal_internal(int *ret_status); #ifdef __COVERITY__ # define ASSERT_SIGNAL(expr, signal) __coverity_check__(((expr), false)) @@ -601,16 +606,19 @@ int assert_signal_internal(void); # define __ASSERT_SIGNAL(uniq, expr, sgnl) \ ({ \ ASSERT_TRUE(SIGNAL_VALID(sgnl)); \ - int UNIQ_T(_r, uniq) = assert_signal_internal(); \ - ASSERT_OK_ERRNO(UNIQ_T(_r, uniq)); \ - if (UNIQ_T(_r, uniq) == 0) { \ + int UNIQ_T(_status, uniq); \ + int UNIQ_T(_path, uniq) = assert_signal_internal(&UNIQ_T(_status, uniq)); \ + ASSERT_OK_ERRNO(UNIQ_T(_path, uniq)); \ + if (UNIQ_T(_path, uniq) == ASSERT_SIGNAL_FORK_CHILD) { \ (void) signal(sgnl, SIG_DFL); \ expr; \ _exit(EXIT_SUCCESS); \ } \ - if (UNIQ_T(_r, uniq) != sgnl) \ + ASSERT_EQ(UNIQ_T(_path, uniq), ASSERT_SIGNAL_FORK_PARENT); \ + if (UNIQ_T(_status, uniq) != sgnl) \ log_test_failed("\"%s\" died with signal %s, but %s was expected", \ - #expr, signal_to_string(UNIQ_T(_r, uniq)), signal_to_string(sgnl)); \ + #expr, signal_to_string(UNIQ_T(_status, uniq)), \ + signal_to_string(sgnl)); \ }) #endif diff --git a/src/test/test-tests.c b/src/test/test-tests.c index 15e36444d3..9a80f8e767 100644 --- a/src/test/test-tests.c +++ b/src/test/test-tests.c @@ -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);