From ce42e3e12d22e61de2891b75dcc7e00bdb534fb4 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 12 Jun 2023 15:40:22 +0100 Subject: [PATCH] Replace g_strsignal() with g_sig2text() This call provides a textual representation of a signal number, i.e. SIGHUP is mapped to "SIGHUP" Unit tests are also added. --- common/string_calls.c | 169 ++++++++++++++++++++++++++----- common/string_calls.h | 40 ++++++-- sesman/sesexec/session.c | 8 +- sesman/sesexec/xwait.c | 9 +- tests/common/test_string_calls.c | 80 +++++++++++++++ xrdp/xrdp_listen.c | 5 +- 6 files changed, 274 insertions(+), 37 deletions(-) diff --git a/common/string_calls.c b/common/string_calls.c index 14448bbf..1ba35613 100644 --- a/common/string_calls.c +++ b/common/string_calls.c @@ -21,6 +21,7 @@ #if defined(HAVE_CONFIG_H) #include "config_ac.h" #endif +#include #include #include #include @@ -422,30 +423,6 @@ g_atoi(const char *str) return atoi(str); } -/*****************************************************************************/ -char * -g_strsignal(int signum) -{ - char *result = strsignal(signum); - - if (result == NULL) - { - // Using a static buffer offers the same guarantees as the - // strsignal() call - static char buff[32]; - unsigned int len; - len = g_snprintf(buff, sizeof(buff), "SIG#%d", signum); - if (len >= sizeof(buff)) - { - // Buffer overflow - g_snprintf(buff, sizeof(buff), "SIG???"); - } - result = buff; - } - - return result; -} - /*****************************************************************************/ /* As g_atoi() but allows for hexadecimal too */ int @@ -1167,3 +1144,147 @@ g_charstr_to_bitmask(const char *str, const struct bitmask_char bitdefs[], return bitmask; } +/*****************************************************************************/ +/* + * Looks for a simple mapping of signal number to name + */ +static const char * +find_sig_name(int signum) +{ + typedef struct + { + int num; + const char *name; + } sig_to_name_type; + + // Map a string 'zzz' to { SIGzzz, "zzz"} for making + // typo-free sig_to_name_type objects +# define DEFSIG(sig) { SIG ## sig, # sig } + + // Entries in this array are taken from + // The Single UNIX ® Specification, Version 2 (1997) + // plus additions from specific operating systems. + // + // The SUS requires these to be positive integer constants with a + // macro definition. Note that SIGRTMIN and SIGRTMAX on Linux are + // NOT constants, so have to be handled separately. + static const sig_to_name_type sigmap[] = + { + // Names from SUS v2, in the order they are listed in that document + // that *should* be defined everywhere + // + // Commented out definitions below are NOT used everywhere + DEFSIG(ABRT), DEFSIG(ALRM), DEFSIG(FPE), DEFSIG(HUP), + DEFSIG(ILL), DEFSIG(INT), DEFSIG(KILL), DEFSIG(PIPE), + DEFSIG(QUIT), DEFSIG(SEGV), DEFSIG(TERM), DEFSIG(USR1), + DEFSIG(USR2), DEFSIG(CHLD), DEFSIG(CONT), DEFSIG(STOP), + DEFSIG(TSTP), DEFSIG(TTIN), DEFSIG(TTOU), DEFSIG(BUS), + /* DEFSIG(POLL), */ /* DEFSIG(PROF), */ DEFSIG(SYS), DEFSIG(TRAP), + DEFSIG(URG), DEFSIG(VTALRM), DEFSIG(XCPU), DEFSIG(XFSZ), + + // SIGPOLL and SIGPROF are marked as obselescent in 1003.1-2017, + // Also SIGPOLL isn't in *BSD operating systems which use SIGIO +#ifdef SIGPOLL + DEFSIG(POLL), +#endif +#ifdef SIGPROF + DEFSIG(PROF), +#endif + + // BSD signals (from FreeBSD/OpenBSD sys/signal.h and + // Darwin/Illumos signal.h) +#ifdef SIGEMT + DEFSIG(EMT), +#endif +#ifdef SIGIO + DEFSIG(IO), +#endif +#ifdef SIGWINCH + DEFSIG(WINCH), +#endif +#ifdef SIGINFO + DEFSIG(INFO), +#endif +#ifdef SIGTHR + DEFSIG(THR), +#endif +#ifdef SIGLIBRT + DEFSIG(LIBRT), +#endif +#ifdef SIGPWR + DEFSIG(PWR), +#endif +#ifdef SIGWAITING + DEFSIG(WAITING), +#endif +#ifdef SIGLWP + DEFSIG(LWP), +#endif + + // Linux additions to *BSD (signal(7)) +#ifdef SIGLOST + DEFSIG(LOST), +#endif +#ifdef SIGSTKFLT + DEFSIG(STKFLT), +#endif + + // Terminator + {0, NULL} +#undef DEFSIG + }; + + const sig_to_name_type *p; + + for (p = &sigmap[0] ; p->name != NULL ; ++p) + { + if (p->num == signum) + { + return p->name; + } + } + + // These aren't constants on Linux +#ifdef SIGRTMIN + if (signum == SIGRTMIN) + { + return "RTMIN"; + } +#endif +#ifdef SIGRTMAX + if (signum == SIGRTMAX) + { + return "RTMAX"; + } +#endif + + return NULL; +} + +/*****************************************************************************/ +char * +g_sig2text(int signum, char sigstr[]) +{ + if (signum >= 0) + { + const char *name = find_sig_name(signum); + + if (name != NULL) + { + g_snprintf(sigstr, MAXSTRSIGLEN, "SIG%s", name); + return sigstr; + } + +#if defined(SIGRTMIN) && defined(SIGRTMAX) + if (signum > SIGRTMIN && signum < SIGRTMAX) + { + g_snprintf(sigstr, MAXSTRSIGLEN, "SIGRTMIN+%d", signum - SIGRTMIN); + return sigstr; + } +#endif + } + + // If all else fails... + g_snprintf(sigstr, MAXSTRSIGLEN, "SIG#%d", signum); + return sigstr; +} diff --git a/common/string_calls.h b/common/string_calls.h index 7403f11b..b4ab8c96 100644 --- a/common/string_calls.h +++ b/common/string_calls.h @@ -67,6 +67,26 @@ struct bitmask_char #define BITMASK_CHAR_END_OF_LIST { 0, '\0' } +enum +{ + // See g_sig2text() + // Must be able to hold "SIG#%d" for INT_MIN + // + // ((sizeof(int) * 5 + 1) / 2) provides a very slight overestimate of + // the bytes requires to store a decimal expansion of 'int':- + // sizeof INT_MAX display bytes ((sizeof(int) * 5 + 1) + // (int) needed / 2) + // ------ ------- ------------- --------------------------- + // 1 127 3 3 + // 2 32767 5 5 + // 3 8388607 7 8 + // 4 2147483637 10 10 + // 8 9*(10**18) 19 20 + // 16 2*(10**38) 39 40 + // 32 6*(10**76) 77 80 + MAXSTRSIGLEN = (3 + 1 + 1 + ((sizeof(int) * 5 + 1) / 2) + 1) +}; + /** * Processes a format string for general info * @@ -266,12 +286,7 @@ int g_strncmp_d(const char *c1, const char *c2, const char delim, int len); int g_strcasecmp(const char *c1, const char *c2); int g_strncasecmp(const char *c1, const char *c2, int len); int g_atoi(const char *str); -/** - * Implements POSIX 1003.1 strsignal() - * - * This function never returns NULL - */ -char *g_strsignal(int signum); + /** * Extends g_atoi(), Converts decimal and hexadecimal number String to integer * @@ -289,4 +304,17 @@ char *g_strstr(const char *haystack, const char *needle); int g_mbstowcs(twchar *dest, const char *src, int n); int g_wcstombs(char *dest, const twchar *src, int n); int g_strtrim(char *str, int trim_flags); + +/** + * Maps a signal number to a string, i.e. SIGHUP -> "SIGHUP" + * + * @param signum Signal number + * @param sigstr buffer for result + * @return sigstr, for convenience + * + * Buffer is assumed to be at least MAXSTRSIGLEN + * + * The string "SIG#" is returned for unrecognised signums + */ +char *g_sig2text(int signum, char sigstr[]); #endif diff --git a/sesman/sesexec/session.c b/sesman/sesexec/session.c index 2385de68..6836d5c4 100644 --- a/sesman/sesexec/session.c +++ b/sesman/sesexec/session.c @@ -821,8 +821,12 @@ exit_status_to_str(const struct exit_status *e, char buff[], int bufflen) break; case E_XR_SIGNAL: - g_snprintf(buff, bufflen, "signal \"%s\"", g_strsignal(e->val)); - break; + { + char sigstr[MAXSTRSIGLEN]; + g_snprintf(buff, bufflen, "signal %s", + g_sig2text(e->val, sigstr)); + } + break; default: g_snprintf(buff, bufflen, "an unexpected error"); diff --git a/sesman/sesexec/xwait.c b/sesman/sesexec/xwait.c index 375399ce..9a7bd376 100644 --- a/sesman/sesexec/xwait.c +++ b/sesman/sesexec/xwait.c @@ -155,10 +155,13 @@ wait_for_xserver(uid_t uid, break; case E_XR_SIGNAL: + { + char sigstr[MAXSTRSIGLEN]; LOG(LOG_LEVEL_ERROR, - "waitforx failed with unexpected signal \"%s\"", - g_strsignal(e.val)); - break; + "waitforx failed with unexpected signal %s", + g_sig2text(e.val, sigstr)); + } + break; default: LOG(LOG_LEVEL_ERROR, diff --git a/tests/common/test_string_calls.c b/tests/common/test_string_calls.c index 0f7d6268..02240809 100644 --- a/tests/common/test_string_calls.c +++ b/tests/common/test_string_calls.c @@ -3,6 +3,10 @@ #include "config_ac.h" #endif +#include +#include + +#include "os_calls.h" #include "string_calls.h" #include "ms-rdpbcgr.h" @@ -1036,6 +1040,76 @@ END_TEST /******************************************************************************/ +START_TEST(test_sigs__common) +{ + char name[MAXSTRSIGLEN]; + char *res; + + // Check some common POSIX signals + res = g_sig2text(SIGHUP, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGHUP"); + + res = g_sig2text(SIGCHLD, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGCHLD"); + + res = g_sig2text(SIGXFSZ, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGXFSZ"); + + res = g_sig2text(SIGRTMIN, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGRTMIN"); + + res = g_sig2text(SIGRTMIN + 2, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGRTMIN+2"); + + res = g_sig2text(SIGRTMAX, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIGRTMAX"); + + // Should be invalid + res = g_sig2text(0, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIG#0"); + + res = g_sig2text(65535, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIG#65535"); + + + // POSIX defines signals as ints, but insists they are positive. So + // we ought to trest we get sane behaviour for -ve numbers + res = g_sig2text(-1, name); + ck_assert_ptr_eq(res, name); + ck_assert_str_eq(res, "SIG#-1"); +} +END_TEST + +START_TEST(test_sigs__bigint) +{ + char name[MAXSTRSIGLEN]; + char name2[1024]; + + // Check that big integers aren't being truncated by the definition + // of MAXSTRSIGLEN + int i = INT_MAX; + + g_sig2text(i, name); + g_snprintf(name2, sizeof(name2), "SIG#%d", i); + ck_assert_str_eq(name, name2); + + i = INT_MIN; + g_sig2text(i, name); + g_snprintf(name2, sizeof(name2), "SIG#%d", i); + ck_assert_str_eq(name, name2); +} +END_TEST + +/******************************************************************************/ + Suite * make_suite_test_string(void) { @@ -1046,6 +1120,7 @@ make_suite_test_string(void) TCase *tc_bm2char; TCase *tc_char2bm; TCase *tc_strtrim; + TCase *tc_sigs; s = suite_create("String"); @@ -1118,5 +1193,10 @@ make_suite_test_string(void) tcase_add_test(tc_strtrim, test_strtrim__trim_both); tcase_add_test(tc_strtrim, test_strtrim__trim_through); + tc_sigs = tcase_create("signals"); + suite_add_tcase(s, tc_sigs); + tcase_add_test(tc_sigs, test_sigs__common); + tcase_add_test(tc_sigs, test_sigs__bigint); + return s; } diff --git a/xrdp/xrdp_listen.c b/xrdp/xrdp_listen.c index d9a6506a..5dd9c19b 100644 --- a/xrdp/xrdp_listen.c +++ b/xrdp/xrdp_listen.c @@ -863,9 +863,10 @@ process_pending_sigchld_events(void) { if (e.reason == E_XR_SIGNAL) { + char sigstr[MAXSTRSIGLEN]; LOG(LOG_LEVEL_ERROR, - "Child %d terminated unexpectedly with signal \"%s\"", - pid, g_strsignal(e.val)); + "Child %d terminated unexpectedly with signal %s", + pid, g_sig2text(e.val, sigstr)); } } }