morgan/systemd
1
0
Fork 0
mirror of https://github.com/morgan9e/systemd synced 2026-04-14 00:14:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

tests: ASSERT_SIGNAL: Prevent hallucinating parent as child and confusing exit codes with signals (#39807)

Browse Source 
This series fixes two distinct, pretty bad bugs in `ASSERT_SIGNAL`.
These bugs can allow failing tests to pass, and can also cause the test
runner to silently terminate prematurely in a way that looks like
success.

This is not theoretical, see
https://github.com/systemd/systemd/pull/39674#discussion_r2540552699 for
a real case of this happening.

---

Bug 1: Parent process hallucinates it is the child and re-executes the
expression being tested

Previously, assert_signal_internal() returned 0 in two mutually
exclusive states:

1. We are the child process (immediately after fork()).
2. We are the parent process, and the child exited normally (status 0).

The macro failed to distinguish these cases. If a child failed to crash
as expected, the parent received 0, incorrectly interpreted it as it
being the child, and re-executed the test expression inside the parent
process.

This can cause tests to falsely pass. The parent would successfully run
the expression (which wasn't supposed to crash in the parent), succeed,
and call _exit(EXIT_SUCCESS).

The second consequence is silent truncation. When the parent called
_exit(), it terminated the entire test runner immediately. Any
subsequent tests in the same binary were never executed.

---

Bug 2: Conflation of exit codes and signals

The harness returned the raw si_status without checking si_code. This
meant that an exit code was indistinguishable from a signal number. For
example, if a child process failed and called exit(6), the harness
reported it as having been killed by SIGABRT (signal 6).

---

This PR both fixes the bugs and reworks the ASSERT_SIGNAL infrastructure
to ensure this is very unlikely to regress:

- assert_signal_internal now returns an explicit control flow enum
(FORK_CHILD / FORK_PARENT) separate from the status data. This makes it
structurally impossible for the parent to hallucinate that it is the
child.
- The output parameter is only populated with a signal number if si_code
confirms the process was killed by a signal. Normal exits return 0.
This commit is contained in:
Chris Down
2025-11-20 03:52:02 +08:00
committed by GitHub
parent 6d36d07599 e21a431ec4
commit 0b7dfc036d
3 changed files with 112 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
src/shared/tests.c
View File

@@ -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;
}

26
src/shared/tests.h
View File

@@ -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

75
src/test/test-tests.c
View File

@@ -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);