diff --git a/src/shared/tests.c b/src/shared/tests.c index 53dda342a4..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,14 +446,23 @@ 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); 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)) + *ret_signal = siginfo.si_status; + else + *ret_signal = 0; + + return ASSERT_SIGNAL_FORK_PARENT; } diff --git a/src/shared/tests.h b/src/shared/tests.h index 4d546c6ab9..31b722829d 100644 --- a/src/shared/tests.h +++ b/src/shared/tests.h @@ -592,23 +592,33 @@ _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)) #else -# define ASSERT_SIGNAL(expr, signal) \ +# define ASSERT_SIGNAL(expr, signal) __ASSERT_SIGNAL(UNIQ, expr, signal) +# define __ASSERT_SIGNAL(uniq, expr, sgnl) \ ({ \ - ASSERT_TRUE(SIGNAL_VALID(signal)); \ - int _r = assert_signal_internal(); \ - ASSERT_OK_ERRNO(_r); \ - if (_r == 0) { \ + ASSERT_TRUE(SIGNAL_VALID(sgnl)); \ + 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 (_r != signal) \ + 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(_r), signal_to_string(signal)); \ + #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 40112a6762..9a80f8e767 100644 --- a/src/test/test-tests.c +++ b/src/test/test-tests.c @@ -137,4 +137,79 @@ 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); +} + +/* 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);