From d11617adbef0d7a31874366e7fad790b7c12e571 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 2 Oct 2023 16:29:06 +0100 Subject: [PATCH 1/2] Remove dependency on signal() function Replaces uses of signal() with sigaction() which should be far more portable. --- common/os_calls.c | 135 +++++++++++++++++++++++++++++--- common/os_calls.h | 51 ++++++++++++ tests/common/Makefile.am | 1 + tests/common/test_common.h | 5 ++ tests/common/test_common_main.c | 5 +- tests/common/test_os_calls.c | 4 + 6 files changed, 191 insertions(+), 10 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index 1afeb5703e..6739a647c4 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -2987,10 +2987,28 @@ g_set_alarm(void (*func)(int), unsigned int secs) #if defined(_WIN32) return 0; #else + struct sigaction action; + /* Cancel any previous alarm to prevent a race */ unsigned int rv = alarm(0); - signal(SIGALRM, func); - (void)alarm(secs); + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGALRM, &action, NULL); + if (func != NULL && secs > 0) + { + (void)alarm(secs); + } return rv; #endif } @@ -3002,7 +3020,22 @@ g_signal_child_stop(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGCHLD, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + // Don't need to know when children are stopped or started + action.sa_flags = (SA_RESTART | SA_NOCLDSTOP); + } + sigemptyset (&action.sa_mask); + + sigaction(SIGCHLD, &action, NULL); #endif } @@ -3013,7 +3046,21 @@ g_signal_segfault(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGSEGV, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESETHAND; // This is a one-shot + } + sigemptyset (&action.sa_mask); + + sigaction(SIGSEGV, &action, NULL); #endif } @@ -3024,7 +3071,21 @@ g_signal_hang_up(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGHUP, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGHUP, &action, NULL); #endif } @@ -3035,7 +3096,21 @@ g_signal_user_interrupt(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGINT, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGINT, &action, NULL); #endif } @@ -3046,7 +3121,21 @@ g_signal_terminate(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGTERM, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGTERM, &action, NULL); #endif } @@ -3057,7 +3146,21 @@ g_signal_pipe(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGPIPE, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGPIPE, &action, NULL); #endif } @@ -3068,7 +3171,21 @@ g_signal_usr1(void (*func)(int)) { #if defined(_WIN32) #else - signal(SIGUSR1, func); + struct sigaction action; + + if (func == NULL) + { + action.sa_handler = SIG_DFL; + action.sa_flags = 0; + } + else + { + action.sa_handler = func; + action.sa_flags = SA_RESTART; + } + sigemptyset (&action.sa_mask); + + sigaction(SIGUSR1, &action, NULL); #endif } diff --git a/common/os_calls.h b/common/os_calls.h index 2d5c18e679..6579d1163a 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -176,6 +176,14 @@ g_sck_get_peer_ip_address(int sck, const char * g_sck_get_peer_description(int sck, char *desc, unsigned int bytes); +/** + * Sleep for the specified number of milli-seconds + * @param msecs Milli-seconds + * + * If a signal is processed, it is possible that this call will + * sleep for less than the specified number of milli-seconds. This + * is platform-specific + */ void g_sleep(int msecs); int g_pipe(int fd[2]); @@ -275,13 +283,56 @@ int g_execvp(const char *p1, char *args[]); */ int g_execvp_list(const char *file, struct list *argv); int g_execlp3(const char *a1, const char *a2, const char *a3); +/** + * Set an alarm using SIGALRM + * @param func Signal handler, or NULL to cancel an alarm + * @param secs Number of seconds until an alarm is raised + * @return Number of seconds remaining before a previously requested + * alarm is raised + */ unsigned int g_set_alarm(void (*func)(int), unsigned int secs); +/** + * Set a handler up for SIGCHLD + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_child_stop(void (*func)(int)); +/** + * Set a handler up for SIGSEGV + * @param func signal handler, or NULL to restore the default handler + * The handler can only be called once, at which point the + * default handler is restored. This is to avoid infinite loops + */ void g_signal_segfault(void (*func)(int)); +/** + * Set a handler up for SIGHUP + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_hang_up(void (*func)(int)); +/** + * Set a handler up for SIGINT + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_user_interrupt(void (*func)(int)); +/** + * Set a handler up for SIGTERM + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_terminate(void (*func)(int)); +/** + * Set a handler up for SIGPIPE + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_pipe(void (*func)(int)); +/** + * Set a handler up for SIGUSR1 + * @param func signal handler, or NULL to restore the default handler + * The handler remains in place until explicitly replaced. + */ void g_signal_usr1(void (*func)(int)); int g_fork(void); int g_setgid(int pid); diff --git a/tests/common/Makefile.am b/tests/common/Makefile.am index de4b0a3c7c..8294afcbd0 100644 --- a/tests/common/Makefile.am +++ b/tests/common/Makefile.am @@ -18,6 +18,7 @@ test_common_SOURCES = \ test_list_calls.c \ test_string_calls.c \ test_os_calls.c \ + test_os_calls_signals.c \ test_ssl_calls.c \ test_base64.c \ test_guid.c diff --git a/tests/common/test_common.h b/tests/common/test_common.h index 09f3acf1d8..ea700c30b8 100644 --- a/tests/common/test_common.h +++ b/tests/common/test_common.h @@ -15,4 +15,9 @@ Suite *make_suite_test_ssl_calls(void); Suite *make_suite_test_base64(void); Suite *make_suite_test_guid(void); +TCase *make_tcase_test_os_calls_signals(void); + +void os_calls_signals_init(void); +void os_calls_signals_deinit(void); + #endif /* TEST_COMMON_H */ diff --git a/tests/common/test_common_main.c b/tests/common/test_common_main.c index ec3f579db8..05eacd2775 100644 --- a/tests/common/test_common_main.c +++ b/tests/common/test_common_main.c @@ -64,14 +64,17 @@ int main (void) * reporting when running in libcheck fork mode */ setvbuf(stdout, NULL, _IONBF, 0); - /* Initialise the ssl module */ + /* Initialise modules */ ssl_init(); + os_calls_signals_init(); srunner_run_all (sr, CK_ENV); number_failed = srunner_ntests_failed(sr); srunner_free(sr); ssl_finish(); + os_calls_signals_deinit(); + log_end(); return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/common/test_os_calls.c b/tests/common/test_os_calls.c index e0f44bbc1b..c2a3e09e8b 100644 --- a/tests/common/test_os_calls.c +++ b/tests/common/test_os_calls.c @@ -512,5 +512,9 @@ make_suite_test_os_calls(void) tcase_add_test(tc_os_calls, test_g_file_is_open); tcase_add_test(tc_os_calls, test_g_sck_fd_passing); tcase_add_test(tc_os_calls, test_g_sck_fd_overflow); + + // Add other test cases in other files + suite_add_tcase(s, make_tcase_test_os_calls_signals()); + return s; } From d0982145314f14291416c02f68c19cf3cec53a34 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 9 Oct 2023 14:04:52 +0100 Subject: [PATCH 2/2] Add tests for signal functions in os_calls.c --- tests/common/test_os_calls_signals.c | 251 +++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 tests/common/test_os_calls_signals.c diff --git a/tests/common/test_os_calls_signals.c b/tests/common/test_os_calls_signals.c new file mode 100644 index 0000000000..1f9745cb27 --- /dev/null +++ b/tests/common/test_os_calls_signals.c @@ -0,0 +1,251 @@ + +#if defined(HAVE_CONFIG_H) +#include "config_ac.h" +#endif + +#include + +#include "os_calls.h" + +#include "test_common.h" + +static tintptr g_wobj1 = 0; + +/******************************************************************************/ +void +os_calls_signals_init(void) +{ + g_wobj1 = g_create_wait_obj(""); +} + +/******************************************************************************/ +void +os_calls_signals_deinit(void) +{ + g_delete_wait_obj(g_wobj1); + g_wobj1 = 0; +} + +/******************************************************************************/ +/** + * Set the global wait object g_wobj1 + */ +static void +set_wobj1(int signum) +{ + g_set_wait_obj(g_wobj1); +} + +/******************************************************************************/ +/** + * Sends a number of signals to the process and checks they are all delivered + * + * @param sig Signal number + * @param count Number of signals to send + * + * The caller is expected to establish a signal handler before this call + * which sets the global g_wobj1 on signal delivery */ + +static +void send_multiple_signals(int sig, unsigned int count) +{ + while (count-- > 0) + { + g_reset_wait_obj(g_wobj1); + ck_assert_int_eq(g_is_wait_obj_set(g_wobj1), 0); + // Expect the signal to be delivered synchronously + raise(sig); + ck_assert_int_ne(g_is_wait_obj_set(g_wobj1), 0); + } +} + +/******************************************************************************/ +START_TEST(test_g_set_alarm) +{ + g_reset_wait_obj(g_wobj1); + ck_assert_int_eq(g_is_wait_obj_set(g_wobj1), 0); + + g_set_alarm(set_wobj1, 1); + + g_obj_wait(&g_wobj1, 1, NULL, 0, 2000); + + ck_assert_int_ne(g_is_wait_obj_set(g_wobj1), 0); + + // Clean up + g_set_alarm(NULL, 0); +} +END_TEST + +/******************************************************************************/ +START_TEST(test_g_signal_child_stop_1) +{ + struct exit_status e; + + g_reset_wait_obj(g_wobj1); + ck_assert_int_eq(g_is_wait_obj_set(g_wobj1), 0); + + g_signal_child_stop(set_wobj1); + + int pid = g_fork(); + + if (pid == 0) + { + g_exit(45); + } + ck_assert_int_ne(pid, 0); + g_obj_wait(&g_wobj1, 1, NULL, 0, 2000); + ck_assert_int_ne(g_is_wait_obj_set(g_wobj1), 0); + + e = g_waitpid_status(pid); + + ck_assert_int_eq(e.reason, E_XR_STATUS_CODE); + ck_assert_int_eq(e.val, 45); + + // Try another one to make sure the signal handler is still in place. + // This one can generate a signal + g_reset_wait_obj(g_wobj1); + + pid = g_fork(); + if (pid == 0) + { + raise(SIGSEGV); + } + ck_assert_int_ne(pid, 0); + g_obj_wait(&g_wobj1, 1, NULL, 0, 2000); + ck_assert_int_ne(g_is_wait_obj_set(g_wobj1), 0); + + e = g_waitpid_status(pid); + + ck_assert_int_eq(e.reason, E_XR_SIGNAL); + ck_assert_int_eq(e.val, SIGSEGV); + + // Clean up + g_signal_child_stop(NULL); +} +END_TEST + +/******************************************************************************/ +/* Checks that multiple children finishing do not interrupt + * g_waitpid_status() */ +START_TEST(test_g_signal_child_stop_2) +{ +#define CHILD_COUNT 20 + int pids[CHILD_COUNT]; + unsigned int i; + + struct exit_status e; + + g_reset_wait_obj(g_wobj1); + ck_assert_int_eq(g_is_wait_obj_set(g_wobj1), 0); + + g_signal_child_stop(set_wobj1); + + for (i = 0 ; i < CHILD_COUNT; ++i) + { + int pid = g_fork(); + if (pid == 0) + { + g_sleep((i + 1) * 100); + g_exit(i + 1); + } + ck_assert_int_ne(pid, 0); + pids[i] = pid; + } + g_obj_wait(&g_wobj1, 1, NULL, 0, 2000); + ck_assert_int_ne(g_is_wait_obj_set(g_wobj1), 0); + + for (i = 0 ; i < CHILD_COUNT; ++i) + { + e = g_waitpid_status(pids[i]); + ck_assert_int_eq(e.reason, E_XR_STATUS_CODE); + ck_assert_int_eq(e.val, (i + 1)); + } + + // Clean up + g_signal_child_stop(NULL); +#undef CHILD_COUNT +} +END_TEST + + +/******************************************************************************/ +START_TEST(test_g_signal_segfault) +{ + g_signal_segfault(set_wobj1); + + // Only one signal can be received in this way. After handling + // the signal the handler should be automatically reset. + send_multiple_signals(SIGSEGV, 1); + + g_signal_segfault(NULL); +} +END_TEST + +/******************************************************************************/ +START_TEST(test_g_signal_hang_up) +{ + g_signal_hang_up(set_wobj1); + + send_multiple_signals(SIGHUP, 5); + + g_signal_hang_up(NULL); +} + +/******************************************************************************/ +START_TEST(test_g_signal_user_interrupt) +{ + g_signal_user_interrupt(set_wobj1); + + send_multiple_signals(SIGINT, 5); + + g_signal_user_interrupt(NULL); +} + +/******************************************************************************/ +START_TEST(test_g_signal_terminate) +{ + g_signal_terminate(set_wobj1); + + send_multiple_signals(SIGTERM, 5); + + g_signal_terminate(NULL); +} + +/******************************************************************************/ +START_TEST(test_g_signal_pipe) +{ + g_signal_pipe(set_wobj1); + + send_multiple_signals(SIGPIPE, 5); + + g_signal_pipe(NULL); +} + +/******************************************************************************/ +START_TEST(test_g_signal_usr1) +{ + g_signal_usr1(set_wobj1); + + send_multiple_signals(SIGUSR1, 5); + + g_signal_usr1(NULL); +} + +/******************************************************************************/ +TCase * +make_tcase_test_os_calls_signals(void) +{ + TCase *tc = tcase_create("oscalls-signals"); + + tcase_add_test(tc, test_g_set_alarm); + tcase_add_test(tc, test_g_signal_child_stop_1); + tcase_add_test(tc, test_g_signal_child_stop_2); + tcase_add_test(tc, test_g_signal_segfault); + tcase_add_test(tc, test_g_signal_hang_up); + tcase_add_test(tc, test_g_signal_user_interrupt); + tcase_add_test(tc, test_g_signal_terminate); + tcase_add_test(tc, test_g_signal_pipe); + tcase_add_test(tc, test_g_signal_usr1); + + return tc; +}