From d3725549f0276487fba1d419094209d18e86669f Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 07:43:57 -0700 Subject: [PATCH 01/18] Add global variable inSignalContext This will be used for checking for proper logging when in signal context. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- hw/xfree86/os-support/shared/sigio.c | 4 ++++ include/globals.h | 3 +++ os/utils.c | 2 ++ 3 files changed, 9 insertions(+) diff --git a/hw/xfree86/os-support/shared/sigio.c b/hw/xfree86/os-support/shared/sigio.c index 12ae8a480..231d6c04f 100644 --- a/hw/xfree86/os-support/shared/sigio.c +++ b/hw/xfree86/os-support/shared/sigio.c @@ -99,6 +99,8 @@ xf86SIGIO(int sig) int save_errno = errno; /* do not clobber the global errno */ int r; + inSignalContext = TRUE; + ready = xf86SigIOMask; to.tv_sec = 0; to.tv_usec = 0; @@ -114,6 +116,8 @@ xf86SIGIO(int sig) } /* restore global errno */ errno = save_errno; + + inSignalContext = FALSE; } static int diff --git a/include/globals.h b/include/globals.h index 8076955a9..6d3f3d708 100644 --- a/include/globals.h +++ b/include/globals.h @@ -2,6 +2,8 @@ #ifndef _XSERV_GLOBAL_H_ #define _XSERV_GLOBAL_H_ +#include + #include "window.h" /* for WindowPtr */ /* Global X server variables that are visible to mi, dix, os, and ddx */ @@ -23,6 +25,7 @@ extern _X_EXPORT int GrabInProgress; extern _X_EXPORT Bool noTestExtensions; extern _X_EXPORT char *SeatId; extern _X_EXPORT char *ConnectionInfo; +extern _X_EXPORT sig_atomic_t inSignalContext; #ifdef DPMSExtension extern _X_EXPORT CARD32 DPMSStandbyTime; diff --git a/os/utils.c b/os/utils.c index 3a1ef9303..2f0754886 100644 --- a/os/utils.c +++ b/os/utils.c @@ -204,6 +204,8 @@ int auditTrailLevel = 1; char *SeatId = NULL; +sig_atomic_t inSignalContext = FALSE; + #if defined(SVR4) || defined(__linux__) || defined(CSRG_BASED) #define HAS_SAVED_IDS_AND_SETEUID #endif From bc85c81687a24aea738094ff11f4448fb3b3afbb Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 08:03:09 -0700 Subject: [PATCH 02/18] Save log file file descriptor for signal context logging None of the FILE based functions are signal safe, including fileno(), so we need to save the file descriptor for when we are in signal context. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- os/log.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/os/log.c b/os/log.c index 2c13c1a7d..5394847aa 100644 --- a/os/log.c +++ b/os/log.c @@ -108,6 +108,7 @@ void (*OsVendorVErrorFProc) (const char *, va_list args) = NULL; #endif static FILE *logFile = NULL; +static int logFileFd = -1; static Bool logFlush = FALSE; static Bool logSync = FALSE; static int logVerbosity = DEFAULT_LOG_VERBOSITY; @@ -211,6 +212,8 @@ LogInit(const char *fname, const char *backup) FatalError("Cannot open log file \"%s\"\n", logFileName); setvbuf(logFile, NULL, _IONBF, 0); + logFileFd = fileno(logFile); + /* Flush saved log information. */ if (saveBuffer && bufferSize > 0) { fwrite(saveBuffer, bufferPos, 1, logFile); @@ -243,6 +246,7 @@ LogClose(enum ExitCode error) (error == EXIT_NO_ERROR) ? "successfully" : "with error", error); fclose(logFile); logFile = NULL; + logFileFd = -1; } } From 704b847abfd29e9adde27127a15a963414f8bcf4 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 10:13:45 -0700 Subject: [PATCH 03/18] Add FormatUInt64{,Hex}() for formatting numbers in a signal safe manner Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- include/misc.h | 2 + os/utils.c | 44 ++++++++++++++++ test/.gitignore | 1 + test/Makefile.am | 3 +- test/signal-logging.c | 115 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 test/signal-logging.c diff --git a/include/misc.h b/include/misc.h index fea74b86c..6ae020a59 100644 --- a/include/misc.h +++ b/include/misc.h @@ -229,6 +229,8 @@ pad_to_int32(const int bytes) } extern char **xstrtokenize(const char *str, const char *separators); +extern void FormatUInt64(uint64_t num, char *string); +extern void FormatUInt64Hex(uint64_t num, char *string); /** * Compare the two version numbers comprising of major.minor. diff --git a/os/utils.c b/os/utils.c index 2f0754886..998c3ed4a 100644 --- a/os/utils.c +++ b/os/utils.c @@ -1793,3 +1793,47 @@ xstrtokenize(const char *str, const char *separators) free(list); return NULL; } + +/* Format a number into a string in a signal safe manner. The string should be + * at least 21 characters in order to handle all uint64_t values. */ +void +FormatUInt64(uint64_t num, char *string) +{ + uint64_t divisor; + int len; + int i; + + for (len = 1, divisor = 10; + len < 20 && num / divisor; + len++, divisor *= 10); + + for (i = len, divisor = 1; i > 0; i--, divisor *= 10) + string[i - 1] = '0' + ((num / divisor) % 10); + + string[len] = '\0'; +} + +/* Format a number into a hexadecimal string in a signal safe manner. The string + * should be at least 17 characters in order to handle all uint64_t values. */ +void +FormatUInt64Hex(uint64_t num, char *string) +{ + uint64_t divisor; + int len; + int i; + + for (len = 1, divisor = 0x10; + len < 16 && num / divisor; + len++, divisor *= 0x10); + + for (i = len, divisor = 1; i > 0; i--, divisor *= 0x10) { + int val = (num / divisor) % 0x10; + + if (val < 10) + string[i - 1] = '0' + val; + else + string[i - 1] = 'a' + val - 10; + } + + string[len] = '\0'; +} diff --git a/test/.gitignore b/test/.gitignore index 5d4fdfa99..27eb2548d 100644 --- a/test/.gitignore +++ b/test/.gitignore @@ -7,3 +7,4 @@ touch xfree86 xkb xtest +signal-logging diff --git a/test/Makefile.am b/test/Makefile.am index b2a53aa64..c049b265d 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -5,7 +5,7 @@ if XORG # Tests that require at least some DDX functions in order to fully link # For now, requires xf86 ddx, could be adjusted to use another SUBDIRS += xi2 -noinst_PROGRAMS += xkb input xtest misc fixes xfree86 hashtabletest +noinst_PROGRAMS += xkb input xtest misc fixes xfree86 hashtabletest signal-logging endif check_LTLIBRARIES = libxservertest.la @@ -36,6 +36,7 @@ misc_LDADD=$(TEST_LDADD) fixes_LDADD=$(TEST_LDADD) xfree86_LDADD=$(TEST_LDADD) touch_LDADD=$(TEST_LDADD) +signal_logging_LDADD=$(TEST_LDADD) hashtabletest_LDADD=$(TEST_LDADD) $(top_srcdir)/Xext/hashtable.c libxservertest_la_LIBADD = $(XSERVER_LIBS) diff --git a/test/signal-logging.c b/test/signal-logging.c new file mode 100644 index 000000000..8aab0dd58 --- /dev/null +++ b/test/signal-logging.c @@ -0,0 +1,115 @@ +/** + * Copyright © 2012 Canonical, Ltd. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#ifdef HAVE_DIX_CONFIG_H +#include +#endif + +#include +#include "assert.h" +#include "misc.h" + +struct number_format_test { + uint64_t number; + char string[21]; + char hex_string[17]; +}; + +static Bool +check_number_format_test(const struct number_format_test *test) +{ + char string[21]; + + FormatUInt64(test->number, string); + if(strncmp(string, test->string, 21) != 0) { + fprintf(stderr, "Failed to convert %ju to decimal string (%s vs %s)\n", + test->number, test->string, string); + return FALSE; + } + FormatUInt64Hex(test->number, string); + if(strncmp(string, test->hex_string, 17) != 0) { + fprintf(stderr, + "Failed to convert %ju to hexadecimal string (%s vs %s)\n", + test->number, test->hex_string, string); + return FALSE; + } + + return TRUE; +} + +static Bool +number_formatting(void) +{ + int i; + struct number_format_test tests[] = { + { /* Zero */ + 0, + "0", + "0", + }, + { /* Single digit number */ + 5, + "5", + "5", + }, + { /* Two digit decimal number */ + 12, + "12", + "c", + }, + { /* Two digit hex number */ + 37, + "37", + "25", + }, + { /* Large < 32 bit number */ + 0xC90B2, + "823474", + "c90b2", + }, + { /* Large > 32 bit number */ + 0x15D027BF211B37A, + "98237498237498234", + "15d027bf211b37a", + }, + { /* Maximum 64-bit number */ + 0xFFFFFFFFFFFFFFFF, + "18446744073709551615", + "ffffffffffffffff", + }, + }; + + for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) + if (!check_number_format_test(tests + i)) + return FALSE; + + return TRUE; +} + +int +main(int argc, char **argv) +{ + int ok = number_formatting(); + + return ok ? 0 : 1; +} From 164b38c72fe9c69d13ea4f9c46d4ccc46566d826 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 08:28:40 -0700 Subject: [PATCH 04/18] Add LogMessageVerbSigSafe() for logging messages while in signal context [whot: edited to use varargs, squashed commit below] Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer os: fix vararg length calculation Make %u and %x sizeof(unsigned int), %p sizeof(void*). This is printf behaviour and we can't guarantee that void* is uint64_t anyway. Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- include/os.h | 7 +++ os/log.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 138 insertions(+), 4 deletions(-) diff --git a/include/os.h b/include/os.h index 276eb5213..b3f9088c5 100644 --- a/include/os.h +++ b/include/os.h @@ -49,6 +49,7 @@ SOFTWARE. #include "misc.h" #include +#include #include #define SCREEN_SAVER_ON 0 @@ -604,6 +605,12 @@ _X_ATTRIBUTE_PRINTF(3, 4); extern _X_EXPORT void LogMessage(MessageType type, const char *format, ...) _X_ATTRIBUTE_PRINTF(2, 3); +extern _X_EXPORT void +LogMessageVerbSigSafe(MessageType type, int verb, const char *format, ...) +_X_ATTRIBUTE_PRINTF(3, 4); +extern _X_EXPORT void +LogVMessageVerbSigSafe(MessageType type, int verb, const char *format, va_list args) +_X_ATTRIBUTE_PRINTF(3, 0); extern _X_EXPORT void LogVHdrMessageVerb(MessageType type, int verb, diff --git a/os/log.c b/os/log.c index 5394847aa..f1a6e3bcd 100644 --- a/os/log.c +++ b/os/log.c @@ -172,6 +172,14 @@ asm(".desc ___crashreporter_info__, 0x10"); #define X_NONE_STRING "" #endif +static size_t +strlen_sigsafe(const char *s) +{ + size_t len; + for (len = 0; s[len]; len++); + return len; +} + /* * LogInit is called to start logging to a file. It is also called (with * NULL arguments) when logging to a file is not wanted. It must always be @@ -271,16 +279,97 @@ LogSetParameter(LogParameter param, int value) } } -/* This function does the actual log message writes. */ +static int +pnprintf(char *string, size_t size, const char *f, va_list args) +{ + int f_idx = 0; + int s_idx = 0; + int f_len = strlen_sigsafe(f); + char *string_arg; + char number[21]; + int p_len; + int i; + uint64_t ui; + + for (; f_idx < f_len && s_idx < size - 1; f_idx++) { + if (f[f_idx] != '%') { + string[s_idx++] = f[f_idx]; + continue; + } + + switch (f[++f_idx]) { + case 's': + string_arg = va_arg(args, char*); + p_len = strlen_sigsafe(string_arg); + + for (i = 0; i < p_len && s_idx < size - 1; i++) + string[s_idx++] = string_arg[i]; + break; + + case 'u': + ui = va_arg(args, unsigned); + FormatUInt64(ui, number); + p_len = strlen_sigsafe(number); + + for (i = 0; i < p_len && s_idx < size - 1; i++) + string[s_idx++] = number[i]; + break; + + case 'p': + string[s_idx++] = '0'; + if (s_idx < size - 1) + string[s_idx++] = 'x'; + ui = (uintptr_t)va_arg(args, void*); + FormatUInt64Hex(ui, number); + p_len = strlen_sigsafe(number); + + for (i = 0; i < p_len && s_idx < size - 1; i++) + string[s_idx++] = number[i]; + break; + + case 'x': + ui = va_arg(args, unsigned); + FormatUInt64Hex(ui, number); + p_len = strlen_sigsafe(number); + + for (i = 0; i < p_len && s_idx < size - 1; i++) + string[s_idx++] = number[i]; + break; + + default: + va_arg(args, char*); + string[s_idx++] = '%'; + if (s_idx < size - 1) + string[s_idx++] = f[f_idx]; + break; + } + } + + string[s_idx] = '\0'; + + return s_idx; +} + +/* This function does the actual log message writes. It must be signal safe. + * When attempting to call non-signal-safe functions, guard them with a check + * of the inSignalContext global variable. */ static void LogSWrite(int verb, const char *buf, size_t len, Bool end_line) { static Bool newline = TRUE; if (verb < 0 || logVerbosity >= verb) - fwrite(buf, len, 1, stderr); + write(2, buf, len); + if (verb < 0 || logFileVerbosity >= verb) { - if (logFile) { + if (inSignalContext && logFileFd >= 0) { + write(logFileFd, buf, len); +#ifdef WIN32 + if (logFlush && logSync) + fsync(logFileFd); +#endif + } + else if (!inSignalContext && logFile) { if (newline) fprintf(logFile, "[%10.3f] ", GetTimeInMillis() / 1000.0); newline = end_line; @@ -293,7 +382,7 @@ LogSWrite(int verb, const char *buf, size_t len, Bool end_line) #endif } } - else if (needBuffer) { + else if (!inSignalContext && needBuffer) { if (len > bufferUnused) { bufferSize += 1024; bufferUnused += 1024; @@ -415,6 +504,44 @@ LogMessage(MessageType type, const char *format, ...) va_end(ap); } +/* Log a message using only signal safe functions. */ +void +LogMessageVerbSigSafe(MessageType type, int verb, const char *format, ...) +{ + va_list ap; + va_start(ap, format); + LogVMessageVerbSigSafe(type, verb, format, ap); + va_end(ap); +} + +void +LogVMessageVerbSigSafe(MessageType type, int verb, const char *format, va_list args) +{ + const char *type_str; + char buf[1024]; + int len; + Bool newline; + + type_str = LogMessageTypeVerbString(type, verb); + if (!type_str) + return; + + /* if type_str is not "", prepend it and ' ', to message */ + if (type_str[0] != '\0') { + LogSWrite(verb, type_str, strlen_sigsafe(type_str), FALSE); + LogSWrite(verb, " ", 1, FALSE); + } + + len = pnprintf(buf, sizeof(buf), format, args); + + /* Force '\n' at end of truncated line */ + if (sizeof(buf) - len == 1) + buf[len - 1] = '\n'; + + newline = (buf[len - 1] == '\n'); + LogSWrite(verb, buf, len, newline); +} + void LogVHdrMessageVerb(MessageType type, int verb, const char *msg_format, va_list msg_args, const char *hdr_format, va_list hdr_args) From ac20815d5235e7a8e7b331365aabf5a489fc5e34 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Tue, 5 Jun 2012 15:39:41 +1000 Subject: [PATCH 05/18] Add ErrorFSigSafe() alternative to ErrorF() ErrorF() is not signal safe. Use ErrorSigSafe() whenever an error message may be logged in signal context. [whot: edited to "ErrorFSigSafe"] Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- include/os.h | 6 ++++++ os/log.c | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/include/os.h b/include/os.h index b3f9088c5..e93c48ae6 100644 --- a/include/os.h +++ b/include/os.h @@ -656,6 +656,12 @@ extern _X_EXPORT void ErrorF(const char *f, ...) _X_ATTRIBUTE_PRINTF(1, 2); extern _X_EXPORT void +VErrorFSigSafe(const char *f, va_list args) +_X_ATTRIBUTE_PRINTF(1, 0); +extern _X_EXPORT void +ErrorFSigSafe(const char *f, ...) +_X_ATTRIBUTE_PRINTF(1, 2); +extern _X_EXPORT void LogPrintMarkers(void); extern _X_EXPORT void diff --git a/os/log.c b/os/log.c index f1a6e3bcd..47ba34876 100644 --- a/os/log.c +++ b/os/log.c @@ -779,6 +779,22 @@ ErrorF(const char *f, ...) va_end(args); } +void +VErrorFSigSafe(const char *f, va_list args) +{ + LogVMessageVerbSigSafe(X_ERROR, -1, f, args); +} + +void +ErrorFSigSafe(const char *f, ...) +{ + va_list args; + + va_start(args, f); + VErrorFSigSafe(f, args); + va_end(args); +} + void LogPrintMarkers(void) { From 0fa5217836cf7fd3872fccc9f3ff9ff32426c25b Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 16:09:05 -0700 Subject: [PATCH 06/18] Print backtrace in a signal-safe manner Backtraces are often printed in signal context, such as when a segfault occurs. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer os: print offset as unsigned int, not long unsigned int pnprintf() takes unsigned int for %u Signed-off-by: Peter Hutterer --- os/backtrace.c | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/os/backtrace.c b/os/backtrace.c index 81348f417..daac60cf6 100644 --- a/os/backtrace.c +++ b/os/backtrace.c @@ -45,29 +45,37 @@ xorg_backtrace(void) int size, i; Dl_info info; - ErrorF("\n"); - ErrorF("Backtrace:\n"); + ErrorFSigSafe("\n"); + ErrorFSigSafe("Backtrace:\n"); size = backtrace(array, 64); for (i = 0; i < size; i++) { int rc = dladdr(array[i], &info); if (rc == 0) { - ErrorF("%d: ?? [%p]\n", i, array[i]); + ErrorFSigSafe("%u: ?? [%p]\n", i, array[i]); continue; } mod = (info.dli_fname && *info.dli_fname) ? info.dli_fname : "(vdso)"; if (info.dli_saddr) - ErrorF("%d: %s (%s+0x%lx) [%p]\n", i, mod, - info.dli_sname, - (long unsigned int) ((char *) array[i] - - (char *) info.dli_saddr), array[i]); + ErrorFSigSafe( + "%u: %s (%s+0x%x) [%p]\n", + i, + mod, + info.dli_sname, + (unsigned int)((char *) array[i] - + (char *) info.dli_saddr), + array[i]); else - ErrorF("%d: %s (%p+0x%lx) [%p]\n", i, mod, - info.dli_fbase, - (long unsigned int) ((char *) array[i] - - (char *) info.dli_fbase), array[i]); + ErrorFSigSafe( + "%u: %s (%p+0x%x) [%p]\n", + i, + mod, + info.dli_fbase, + (unsigned int)((char *) array[i] - + (char *) info.dli_fbase), + array[i]); } - ErrorF("\n"); + ErrorFSigSafe("\n"); } #else /* not glibc or glibc < 2.1 */ @@ -105,7 +113,7 @@ xorg_backtrace_frame(uintptr_t pc, int signo, void *arg) strcpy(signame, "unknown"); } - ErrorF("** Signal %d (%s)\n", signo, signame); + ErrorFSigSafe("** Signal %u (%s)\n", signo, signame); } snprintf(header, sizeof(header), "%d: 0x%lx", depth, pc); @@ -123,7 +131,8 @@ xorg_backtrace_frame(uintptr_t pc, int signo, void *arg) symname = "
"; offset = pc - (uintptr_t) dlinfo.dli_fbase; } - ErrorF("%s: %s:%s+0x%lx\n", header, dlinfo.dli_fname, symname, offset); + ErrorFSigSafe("%s: %s:%s+0x%x\n", header, dlinfo.dli_fname, symname, + offset); } else { @@ -131,7 +140,7 @@ xorg_backtrace_frame(uintptr_t pc, int signo, void *arg) * probably poke elfloader here, but haven't written that code yet, * so we just print the pc. */ - ErrorF("%s\n", header); + ErrorFSigSafe("%s\n", header); } return 0; @@ -183,7 +192,7 @@ xorg_backtrace_pstack(void) if (bytesread > 0) { btline[bytesread] = 0; - ErrorF("%s", btline); + ErrorFSigSafe("%s", btline); } else if ((bytesread < 0) || ((errno != EINTR) && (errno != EAGAIN))) done = 1; @@ -203,8 +212,8 @@ void xorg_backtrace(void) { - ErrorF("\n"); - ErrorF("Backtrace:\n"); + ErrorFSigSafe("\n"); + ErrorFSigSafe("Backtrace:\n"); #ifdef HAVE_PSTACK /* First try fork/exec of pstack - otherwise fall back to walkcontext @@ -221,9 +230,9 @@ xorg_backtrace(void) walkcontext(&u, xorg_backtrace_frame, &depth); else #endif - ErrorF("Failed to get backtrace info: %s\n", strerror(errno)); + ErrorFSigSafe("Failed to get backtrace info: %s\n", strerror(errno)); } - ErrorF("\n"); + ErrorFSigSafe("\n"); } #else From 512bec06be6c79ca263da9de8f40430b8095b57b Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Mon, 16 Apr 2012 09:47:42 -0700 Subject: [PATCH 07/18] Make BUG_WARN* signal safe Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- include/misc.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/misc.h b/include/misc.h index 6ae020a59..aa62f6a3a 100644 --- a/include/misc.h +++ b/include/misc.h @@ -371,10 +371,10 @@ extern _X_EXPORT unsigned long serverGeneration; /* Don't use this directly, use BUG_WARN or BUG_WARN_MSG instead */ #define __BUG_WARN_MSG(cond, with_msg, ...) \ do { if (cond) { \ - ErrorF("BUG: triggered 'if (" #cond ")'\n"); \ - ErrorF("BUG: %s:%d in %s()\n", \ - __FILE__, __LINE__, __func__); \ - if (with_msg) ErrorF(__VA_ARGS__); \ + ErrorFSigSafe("BUG: triggered 'if (" #cond ")'\n"); \ + ErrorFSigSafe("BUG: %s:%u in %s()\n", \ + __FILE__, __LINE__, __func__); \ + if (with_msg) ErrorFSigSafe(__VA_ARGS__); \ xorg_backtrace(); \ } } while(0) From 82d1c6b310eaa5095eed9ee4ea958261a46a78e1 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 08:32:28 -0700 Subject: [PATCH 08/18] Warn when attempting to log in a signal unsafe manner from signal context Also, print out the offending message format. This will hopefully help developers track down unsafe logging. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- os/log.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/os/log.c b/os/log.c index 47ba34876..25da9f63a 100644 --- a/os/log.c +++ b/os/log.c @@ -463,6 +463,16 @@ LogVMessageVerb(MessageType type, int verb, const char *format, va_list args) Bool newline; size_t len = 0; + if (inSignalContext) { + BUG_WARN_MSG(inSignalContext, + "Warning: attempting to log data in a signal unsafe " + "manner while in signal context. Please update to check " + "inSignalContext and/or use LogMessageVerbSigSafe() or " + "ErrorFSigSafe(). The offending log format message is:\n" + "%s\n", format); + return; + } + type_str = LogMessageTypeVerbString(type, verb); if (!type_str) return; @@ -552,6 +562,16 @@ LogVHdrMessageVerb(MessageType type, int verb, const char *msg_format, Bool newline; size_t len = 0; + if (inSignalContext) { + BUG_WARN_MSG(inSignalContext, + "Warning: attempting to log data in a signal unsafe " + "manner while in signal context. Please update to check " + "inSignalContext and/or use LogMessageVerbSigSafe(). The " + "offending header and log message formats are:\n%s %s\n", + hdr_format, msg_format); + return; + } + type_str = LogMessageTypeVerbString(type, verb); if (!type_str) return; From f752226e40890643df213a62f0c96e6a0243e754 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 10:21:14 -0700 Subject: [PATCH 09/18] Log messages in GetTouchEvents() in a signal safe manner Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- dix/getevents.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dix/getevents.c b/dix/getevents.c index baa26c4b8..3fa3a1e3e 100644 --- a/dix/getevents.c +++ b/dix/getevents.c @@ -1839,8 +1839,8 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid, touchpoint.ti = TouchFindByDDXID(dev, ddx_touchid, (type == XI_TouchBegin)); if (!touchpoint.ti) { - ErrorF("[dix] %s: unable to %s touch point %x\n", dev->name, - type == XI_TouchBegin ? "begin" : "find", ddx_touchid); + ErrorFSigSafe("[dix] %s: unable to %s touch point %u\n", dev->name, + type == XI_TouchBegin ? "begin" : "find", ddx_touchid); return 0; } client_id = touchpoint.ti->client_id; From 7f4a69b628a6246855054a0b94d6d6dd14e8842c Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 13 Apr 2012 16:01:38 -0700 Subject: [PATCH 10/18] Log messages in TouchBeginDDXTouch() in a signal-safe manner Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- dix/touch.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dix/touch.c b/dix/touch.c index aa17faf28..a01f152cd 100644 --- a/dix/touch.c +++ b/dix/touch.c @@ -198,8 +198,9 @@ TouchBeginDDXTouch(DeviceIntPtr dev, uint32_t ddx_id) /* If we get here, then we've run out of touches and we need to drop the * event (we're inside the SIGIO handler here) schedule a WorkProc to * grow the queue for us for next time. */ - ErrorF("%s: not enough space for touch events (max %d touchpoints). " - "Dropping this event.\n", dev->name, dev->last.num_touches); + ErrorFSigSafe("%s: not enough space for touch events (max %u touchpoints). " + "Dropping this event.\n", dev->name, dev->last.num_touches); + if (!BitIsOn(resize_waiting, dev->id)) { SetBit(resize_waiting, dev->id); QueueWorkProc(TouchResizeQueue, serverClient, NULL); From 6fd5add005d0660b591d808583d1a6c6a85f1277 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Fri, 6 Apr 2012 16:17:41 -0700 Subject: [PATCH 11/18] Log mieq enqueue overflow in a signal safe manner Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- mi/mieq.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/mi/mieq.c b/mi/mieq.c index e117a8db7..b2c7769ec 100644 --- a/mi/mieq.c +++ b/mi/mieq.c @@ -276,23 +276,22 @@ mieqEnqueue(DeviceIntPtr pDev, InternalEvent *e) */ miEventQueue.dropped++; if (miEventQueue.dropped == 1) { - ErrorF - ("[mi] EQ overflowing. Additional events will be discarded until existing events are processed.\n"); + ErrorFSigSafe("[mi] EQ overflowing. Additional events will be " + "discarded until existing events are processed.\n"); xorg_backtrace(); - ErrorF - ("[mi] These backtraces from mieqEnqueue may point to a culprit higher up the stack.\n"); - ErrorF("[mi] mieq is *NOT* the cause. It is a victim.\n"); + ErrorFSigSafe("[mi] These backtraces from mieqEnqueue may point to " + "a culprit higher up the stack.\n"); + ErrorFSigSafe("[mi] mieq is *NOT* the cause. It is a victim.\n"); } else if (miEventQueue.dropped % QUEUE_DROP_BACKTRACE_FREQUENCY == 0 && miEventQueue.dropped / QUEUE_DROP_BACKTRACE_FREQUENCY <= QUEUE_DROP_BACKTRACE_MAX) { - ErrorF - ("[mi] EQ overflow continuing. %lu events have been dropped.\n", - miEventQueue.dropped); + ErrorFSigSafe("[mi] EQ overflow continuing. %u events have been " + "dropped.\n", miEventQueue.dropped); if (miEventQueue.dropped / QUEUE_DROP_BACKTRACE_FREQUENCY == QUEUE_DROP_BACKTRACE_MAX) { - ErrorF - ("[mi] No further overflow reports will be reported until the clog is cleared.\n"); + ErrorFSigSafe("[mi] No further overflow reports will be " + "reported until the clog is cleared.\n"); } xorg_backtrace(); } From 89e3ac07aca1def155299aff6f7a57ccafb68fd7 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Mon, 9 Apr 2012 08:23:32 -0700 Subject: [PATCH 12/18] Log safely in fatal signal handler While we probably don't need to be signal safe here since we will never return to the normal context, the logging signal context check will cause unsafe logging to be unhandled. Using signal safe logging here resolves the issue. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- os/osinit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/os/osinit.c b/os/osinit.c index e2a220886..6cc040178 100644 --- a/os/osinit.c +++ b/os/osinit.c @@ -113,7 +113,7 @@ OsSigHandler(int signo) const char *dlerr = dlerror(); if (dlerr) { - LogMessage(X_ERROR, "Dynamic loader error: %s\n", dlerr); + LogMessageVerbSigSafe(X_ERROR, 1, "Dynamic loader error: %s\n", dlerr); } #endif /* RTLD_DI_SETSIGNAL */ @@ -129,8 +129,8 @@ OsSigHandler(int signo) #ifdef SA_SIGINFO if (sip->si_code == SI_USER) { - ErrorF("Recieved signal %d sent by process %ld, uid %ld\n", - signo, (long) sip->si_pid, (long) sip->si_uid); + ErrorFSigSafe("Recieved signal %u sent by process %u, uid %u\n", signo, + sip->si_pid, sip->si_uid); } else { switch (signo) { @@ -138,7 +138,7 @@ OsSigHandler(int signo) case SIGBUS: case SIGILL: case SIGFPE: - ErrorF("%s at address %p\n", strsignal(signo), sip->si_addr); + ErrorFSigSafe("%s at address %p\n", strsignal(signo), sip->si_addr); } } #endif From c3e1168778ec20beeac9979dc57e36400c70dd63 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Mon, 9 Apr 2012 08:28:17 -0700 Subject: [PATCH 13/18] Log in UnloadModuleOrDriver() in a signal safe manner The function may be called from a fatal signal handler. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- hw/xfree86/loader/loadmod.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c index 706b9b3e8..72020a58c 100644 --- a/hw/xfree86/loader/loadmod.c +++ b/hw/xfree86/loader/loadmod.c @@ -1093,9 +1093,10 @@ UnloadModuleOrDriver(ModuleDescPtr mod) return; if (mod->parent) - xf86MsgVerb(X_INFO, 3, "UnloadSubModule: \"%s\"\n", mod->name); + LogMessageVerbSigSafe(X_INFO, 3, "UnloadSubModule: \"%s\"\n", + mod->name); else - xf86MsgVerb(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name); + LogMessageVerbSigSafe(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name); if (mod->TearDownData != ModuleDuplicated) { if ((mod->TearDownProc) && (mod->TearDownData)) From d51aebdbf99a9f240f7c318a70ba40e61cd43049 Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Mon, 9 Apr 2012 08:30:50 -0700 Subject: [PATCH 14/18] Log in LoaderUnload() in a signal safe manner The function may be called from a fatal signal handler. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- hw/xfree86/loader/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/loader/loader.c b/hw/xfree86/loader/loader.c index b72b8b89d..edaefb8f9 100644 --- a/hw/xfree86/loader/loader.c +++ b/hw/xfree86/loader/loader.c @@ -163,7 +163,7 @@ LoaderSymbol(const char *name) void LoaderUnload(const char *name, void *handle) { - xf86Msg(X_INFO, "Unloading %s\n", name); + LogMessageVerbSigSafe(X_INFO, 1, "Unloading %s\n", name); if (handle) dlclose(handle); } From 505c8a2b2cae0318db1148417ec850d54b38f7df Mon Sep 17 00:00:00 2001 From: Chase Douglas Date: Mon, 9 Apr 2012 09:41:38 -0700 Subject: [PATCH 15/18] Log in OsVendorFatalError() in a signal safe manner The function can be called from a fatal signal handler. Signed-off-by: Chase Douglas Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- hw/xfree86/common/xf86Init.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c index ca6efd44e..84c866944 100644 --- a/hw/xfree86/common/xf86Init.c +++ b/hw/xfree86/common/xf86Init.c @@ -1058,16 +1058,16 @@ void OsVendorFatalError(const char *f, va_list args) { #ifdef VENDORSUPPORT - ErrorF("\nPlease refer to your Operating System Vendor support pages\n" - "at %s for support on this crash.\n", VENDORSUPPORT); + ErrorFSigSafe("\nPlease refer to your Operating System Vendor support " + "pages\nat %s for support on this crash.\n", VENDORSUPPORT); #else - ErrorF("\nPlease consult the " XVENDORNAME " support \n" - "\t at " __VENDORDWEBSUPPORT__ "\n for help. \n"); + ErrorFSigSafe("\nPlease consult the " XVENDORNAME " support \n\t at " + __VENDORDWEBSUPPORT__ "\n for help. \n"); #endif if (xf86LogFile && xf86LogFileWasOpened) - ErrorF("Please also check the log file at \"%s\" for additional " - "information.\n", xf86LogFile); - ErrorF("\n"); + ErrorFSigSafe("Please also check the log file at \"%s\" for additional " + "information.\n", xf86LogFile); + ErrorFSigSafe("\n"); } int From c66089d2206bafc01307a8327ff6089edcb4ed2d Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 28 May 2012 09:57:33 +1000 Subject: [PATCH 16/18] xfree86: constify InputInfoPtr->type_name This corresponds to XListInputDevice(3)'s "type" field (after being converted to an Atom). Input drivers use the XI_KEYBOARD and similar defines, even Wacom which falls out of the common defines uses constant strings here. The use-case for having this non-const is small. Input ABI break technically, since we never freed this information anyway it is not a noticable change. Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas Signed-off-by: Peter Hutterer --- hw/xfree86/common/xf86Xinput.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Xinput.h b/hw/xfree86/common/xf86Xinput.h index 1d4363a50..6ccccf135 100644 --- a/hw/xfree86/common/xf86Xinput.h +++ b/hw/xfree86/common/xf86Xinput.h @@ -98,7 +98,7 @@ typedef struct _InputInfoRec { int fd; DeviceIntPtr dev; pointer private; - char *type_name; + const char *type_name; InputDriverPtr drv; pointer module; XF86OptionPtr options; From 541934168dbeb17059542bb5a1da8eba7995fa05 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 28 May 2012 10:10:30 +1000 Subject: [PATCH 17/18] xfree86: constify InputDriverPtr->driverName and default_options Already treated as const anyway by all drivers. Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas Signed-off-by: Peter Hutterer --- hw/xfree86/common/xf86Xinput.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/xfree86/common/xf86Xinput.h b/hw/xfree86/common/xf86Xinput.h index 6ccccf135..35c38a554 100644 --- a/hw/xfree86/common/xf86Xinput.h +++ b/hw/xfree86/common/xf86Xinput.h @@ -68,14 +68,14 @@ /* This holds the input driver entry and module information. */ typedef struct _InputDriverRec { int driverVersion; - char *driverName; + const char *driverName; void (*Identify) (int flags); int (*PreInit) (struct _InputDriverRec * drv, struct _InputInfoRec * pInfo, int flags); void (*UnInit) (struct _InputDriverRec * drv, struct _InputInfoRec * pInfo, int flags); pointer module; - char **default_options; + const char **default_options; } InputDriverRec, *InputDriverPtr; /* This is to input devices what the ScrnInfoRec is to screens. */ From 35e3d229150395a222a0f53318daf5dbeb8f6eb6 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 24 May 2012 14:04:42 +1000 Subject: [PATCH 18/18] Bump to ABI_XINPUT_VERSION 18 The input ABI hasn't changed, but input drivers need something to hook on if they want to log from within signal handlers and the input ABI is the simplest way of doing so. Signed-off-by: Peter Hutterer --- hw/xfree86/common/xf86Module.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index bf56acd05..7671cea5d 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -83,7 +83,7 @@ typedef enum { */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) #define ABI_VIDEODRV_VERSION SET_ABI_VERSION(13, 0) -#define ABI_XINPUT_VERSION SET_ABI_VERSION(17, 0) +#define ABI_XINPUT_VERSION SET_ABI_VERSION(18, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(6, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6)