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.

diff --git a/src/shared/tests.c b/src/shared/tests.c
index 9dc0980025e0d2dc211a882d8b0b8b931ea34063..442fe83045f8781e0141d934a3946887bfcde9e4 100644 (file)
--- 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 2aced04265257dd066513febde7eadae03a9c0b9..31b722829d67b782f0d4d84f26eeed7d88919188 100644 (file)
--- 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 15e36444d3b27177f0918f711ba0566442cfa73c..9a80f8e76796e890206b11c01457198fd8a43e19 100644 (file)
--- 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);