From b69536b475118a8787c1726355b504207bf83f8f Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 13 Aug 2012 11:15:54 +1000 Subject: [PATCH 1/8] test: assert from signal-safe number conversion Throw an assert when the conversion fails instead of just returning. Asserts are more informative. Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- test/signal-logging.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/signal-logging.c b/test/signal-logging.c index 8aab0dd58..646715033 100644 --- a/test/signal-logging.c +++ b/test/signal-logging.c @@ -57,7 +57,7 @@ check_number_format_test(const struct number_format_test *test) return TRUE; } -static Bool +static void number_formatting(void) { int i; @@ -100,16 +100,14 @@ number_formatting(void) }; for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) - if (!check_number_format_test(tests + i)) - return FALSE; + assert(check_number_format_test(tests + i)); - return TRUE; } int main(int argc, char **argv) { - int ok = number_formatting(); + number_formatting(); - return ok ? 0 : 1; + return 0; } From 36c1d92ec0ef0f3927034a12d4cb79dcc22bd185 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 13 Aug 2012 12:24:39 +1000 Subject: [PATCH 2/8] test: add a few tests for signal-safe logging Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- test/signal-logging.c | 101 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/test/signal-logging.c b/test/signal-logging.c index 646715033..f66f773c4 100644 --- a/test/signal-logging.c +++ b/test/signal-logging.c @@ -26,6 +26,7 @@ #endif #include +#include #include "assert.h" #include "misc.h" @@ -101,13 +102,113 @@ number_formatting(void) for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) assert(check_number_format_test(tests + i)); +} +static void logging_format(void) +{ + const char *log_file_path = "/tmp/Xorg-logging-test.log"; + const char *str = "%s %d %u %% %p %i"; + char buf[1024]; + int i; + unsigned int ui; + FILE *f; + char read_buf[2048]; + char *logmsg; + uintptr_t ptr; + + /* set up buf to contain ".....end" */ + memset(buf, '.', sizeof(buf)); + strcpy(&buf[sizeof(buf) - 4], "end"); + + LogInit(log_file_path, NULL); + assert(f = fopen(log_file_path, "r")); + +#define read_log_msg(msg) \ + fgets(read_buf, sizeof(read_buf), f); \ + msg = strchr(read_buf, ']') + 2; /* advance past [time.stamp] */ + + /* boring test message */ + LogMessageVerbSigSafe(X_ERROR, -1, "test message\n"); + read_log_msg(logmsg); + assert(strcmp(logmsg, "(EE) test message\n") == 0); + + /* long buf is truncated to "....en\n" */ +#pragma GCC diagnostic ignored "-Wformat-security" + LogMessageVerbSigSafe(X_ERROR, -1, buf); +#pragma GCC diagnostic pop "-Wformat-security" + read_log_msg(logmsg); + assert(strcmp(&logmsg[strlen(logmsg) - 3], "en\n") == 0); + + /* same thing, this time as string substitution */ + LogMessageVerbSigSafe(X_ERROR, -1, "%s", buf); + read_log_msg(logmsg); + assert(strcmp(&logmsg[strlen(logmsg) - 3], "en\n") == 0); + + /* strings containing placeholders should just work */ + LogMessageVerbSigSafe(X_ERROR, -1, "%s\n", str); + read_log_msg(logmsg); + assert(strcmp(logmsg, "(EE) %s %d %u %% %p %i\n") == 0); + + /* string substitution */ + LogMessageVerbSigSafe(X_ERROR, -1, "%s\n", "substituted string"); + read_log_msg(logmsg); + assert(strcmp(logmsg, "(EE) substituted string\n") == 0); + + /* number substitution */ + ui = 0; + do { + char expected[30]; + sprintf(expected, "(EE) %u\n", ui); + LogMessageVerbSigSafe(X_ERROR, -1, "%u\n", ui); + read_log_msg(logmsg); + assert(strcmp(logmsg, expected) == 0); + if (ui == 0) + ui = 1; + else + ui <<= 1; + } while(ui); + + /* hex number substitution */ + ui = 0; + do { + char expected[30]; + sprintf(expected, "(EE) %x\n", ui); + LogMessageVerbSigSafe(X_ERROR, -1, "%x\n", ui); + read_log_msg(logmsg); + assert(strcmp(logmsg, expected) == 0); + if (ui == 0) + ui = 1; + else + ui <<= 1; + } while(ui); + + /* pointer substitution */ + /* we print a null-pointer differently to printf */ + LogMessageVerbSigSafe(X_ERROR, -1, "%p\n", NULL); + read_log_msg(logmsg); + assert(strcmp(logmsg, "(EE) 0x0\n") == 0); + + ptr = 1; + do { + char expected[30]; + sprintf(expected, "(EE) %p\n", (void*)ptr); + LogMessageVerbSigSafe(X_ERROR, -1, "%p\n", (void*)ptr); + read_log_msg(logmsg); + assert(strcmp(logmsg, expected) == 0); + ptr <<= 1; + } while(ptr); + + LogClose(EXIT_NO_ERROR); + unlink(log_file_path); + +#undef read_log_msg } int main(int argc, char **argv) { number_formatting(); + logging_format(); return 0; } From 7f8c39c8b5ef89153ecd84d16331e96d8feb18ef Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 13 Aug 2012 14:24:36 +1000 Subject: [PATCH 3/8] Add FormatInt64 to convert signed integers in signal-safe manner Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- include/misc.h | 1 + os/utils.c | 14 ++++++++ test/signal-logging.c | 80 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 92 insertions(+), 3 deletions(-) diff --git a/include/misc.h b/include/misc.h index 7f7f221a8..348717676 100644 --- a/include/misc.h +++ b/include/misc.h @@ -247,6 +247,7 @@ padding_for_int32(const int bytes) extern char **xstrtokenize(const char *str, const char *separators); +extern void FormatInt64(int64_t num, char *string); extern void FormatUInt64(uint64_t num, char *string); extern void FormatUInt64Hex(uint64_t num, char *string); diff --git a/os/utils.c b/os/utils.c index 947f8673a..04bcbc61f 100644 --- a/os/utils.c +++ b/os/utils.c @@ -1924,6 +1924,20 @@ xstrtokenize(const char *str, const char *separators) return NULL; } +/* Format a signed number into a string in a signal safe manner. The string + * should be at least 21 characters in order to handle all int64_t values. + */ +void +FormatInt64(int64_t num, char *string) +{ + if (num < 0) { + string[0] = '-'; + num *= -1; + string++; + } + FormatUInt64(num, string); +} + /* 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 diff --git a/test/signal-logging.c b/test/signal-logging.c index f66f773c4..0e352aa0b 100644 --- a/test/signal-logging.c +++ b/test/signal-logging.c @@ -36,6 +36,26 @@ struct number_format_test { char hex_string[17]; }; +struct signed_number_format_test { + int64_t number; + char string[21]; +}; + +static Bool +check_signed_number_format_test(const struct signed_number_format_test *test) +{ + char string[21]; + + FormatInt64(test->number, string); + if(strncmp(string, test->string, 21) != 0) { + fprintf(stderr, "Failed to convert %jd to decimal string (%s vs %s)\n", + test->number, test->string, string); + return FALSE; + } + + return TRUE; +} + static Bool check_number_format_test(const struct number_format_test *test) { @@ -62,7 +82,7 @@ static void number_formatting(void) { int i; - struct number_format_test tests[] = { + struct number_format_test unsigned_tests[] = { { /* Zero */ 0, "0", @@ -100,8 +120,62 @@ number_formatting(void) }, }; - for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) - assert(check_number_format_test(tests + i)); + struct signed_number_format_test signed_tests[] = { + { /* Zero */ + 0, + "0", + }, + { /* Single digit number */ + 5, + "5", + }, + { /* Two digit decimal number */ + 12, + "12", + }, + { /* Two digit hex number */ + 37, + "37", + }, + { /* Large < 32 bit number */ + 0xC90B2, + "823474", + }, + { /* Large > 32 bit number */ + 0x15D027BF211B37A, + "98237498237498234", + }, + { /* Maximum 64-bit signed number */ + 0x7FFFFFFFFFFFFFFF, + "9223372036854775807", + }, + { /* Single digit number */ + -1, + "-1", + }, + { /* Two digit decimal number */ + -12, + "-12", + }, + { /* Large < 32 bit number */ + -0xC90B2, + "-823474", + }, + { /* Large > 32 bit number */ + -0x15D027BF211B37A, + "-98237498237498234", + }, + { /* Maximum 64-bit number */ + -0x7FFFFFFFFFFFFFFF, + "-9223372036854775807", + }, + }; + + for (i = 0; i < sizeof(unsigned_tests) / sizeof(unsigned_tests[0]); i++) + assert(check_number_format_test(unsigned_tests + i)); + + for (i = 0; i < sizeof(unsigned_tests) / sizeof(signed_tests[0]); i++) + assert(check_signed_number_format_test(signed_tests + i)); } static void logging_format(void) From 4912b4adb666dad96b832ab2d7caaae49808723e Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 13 Aug 2012 14:44:44 +1000 Subject: [PATCH 4/8] os: add support for %d and %i to pnprintf The mouse driver uses %i in some debug messages Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- os/log.c | 10 ++++++++++ test/signal-logging.c | 21 +++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/os/log.c b/os/log.c index 25da9f63a..4348739d4 100644 --- a/os/log.c +++ b/os/log.c @@ -290,6 +290,7 @@ pnprintf(char *string, size_t size, const char *f, va_list args) int p_len; int i; uint64_t ui; + int64_t si; for (; f_idx < f_len && s_idx < size - 1; f_idx++) { if (f[f_idx] != '%') { @@ -311,6 +312,15 @@ pnprintf(char *string, size_t size, const char *f, va_list args) 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 'i': + case 'd': + si = va_arg(args, int); + FormatInt64(si, number); + p_len = strlen_sigsafe(number); + for (i = 0; i < p_len && s_idx < size - 1; i++) string[s_idx++] = number[i]; break; diff --git a/test/signal-logging.c b/test/signal-logging.c index 0e352aa0b..3206ddefa 100644 --- a/test/signal-logging.c +++ b/test/signal-logging.c @@ -242,6 +242,27 @@ static void logging_format(void) ui <<= 1; } while(ui); + /* signed number substitution */ + i = 0; + do { + char expected[30]; + sprintf(expected, "(EE) %d\n", i); + LogMessageVerbSigSafe(X_ERROR, -1, "%d\n", i); + read_log_msg(logmsg); + assert(strcmp(logmsg, expected) == 0); + + + sprintf(expected, "(EE) %d\n", i | INT_MIN); + LogMessageVerbSigSafe(X_ERROR, -1, "%d\n", i | INT_MIN); + read_log_msg(logmsg); + assert(strcmp(logmsg, expected) == 0); + + if (i == 0) + i = 1; + else + i <<= 1; + } while(i > INT_MIN); + /* hex number substitution */ ui = 0; do { From 1ebba43052d68d874148e63c9ae38489ddfc5ec1 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Wed, 15 Aug 2012 14:49:04 +1000 Subject: [PATCH 5/8] os: don't block signal-unsafe logging, merely warn about it. Throw an error into the log file, but continue anyway. And after three warnings, stop complaining. Not all input drivers will be fixed in time (or ever) and our printf implementation is vastly inferior, so there is still a use-case for non-sigsafe logging. This also adds more linebreaks to the message. CC: Chase Douglas Signed-off-by: Peter Hutterer Reviewed-by: Chase Douglas --- os/log.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/os/log.c b/os/log.c index 4348739d4..a0628fda3 100644 --- a/os/log.c +++ b/os/log.c @@ -467,6 +467,7 @@ LogMessageTypeVerbString(MessageType type, int verb) void LogVMessageVerb(MessageType type, int verb, const char *format, va_list args) { + static unsigned int warned; const char *type_str; char buf[1024]; const size_t size = sizeof(buf); @@ -474,13 +475,17 @@ LogVMessageVerb(MessageType type, int verb, const char *format, va_list args) 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; + if (warned < 3) { + BUG_WARN_MSG(inSignalContext, + "Warning: attempting to log data in a signal unsafe " + "manner while in signal context.\nPlease update to check " + "inSignalContext and/or use LogMessageVerbSigSafe() or " + "ErrorFSigSafe().\nThe offending log format message is:\n" + "%s\n", format); + warned++; + if (warned == 3) + LogMessageVerbSigSafe(X_WARNING, -1, "Warned %u times about sigsafe logging. Will be quiet now.\n", warned); + } } type_str = LogMessageTypeVerbString(type, verb); @@ -566,6 +571,7 @@ void LogVHdrMessageVerb(MessageType type, int verb, const char *msg_format, va_list msg_args, const char *hdr_format, va_list hdr_args) { + static unsigned int warned; const char *type_str; char buf[1024]; const size_t size = sizeof(buf); @@ -573,13 +579,17 @@ LogVHdrMessageVerb(MessageType type, int verb, const char *msg_format, 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; + if (warned < 3) { + BUG_WARN_MSG(inSignalContext, + "Warning: attempting to log data in a signal unsafe " + "manner while in signal context.\nPlease update to check " + "inSignalContext and/or use LogMessageVerbSigSafe().\nThe " + "offending header and log message formats are:\n%s %s\n", + hdr_format, msg_format); + warned++; + if (warned == 3) + LogMessageVerbSigSafe(X_WARNING, -1, "Warned %u times about sigsafe logging. Will be quiet now.\n", warned); + } } type_str = LogMessageTypeVerbString(type, verb); From bafbd99080be49a17be97d2cc758fbe623369945 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Wed, 8 Aug 2012 11:34:32 +1000 Subject: [PATCH 6/8] dix: work around scaling issues during WarpPointer (#53037) In WarpPointer calls, we get input in screen coordinates. They must be scaled to device coordinates, and then back to screen coordinates for screen crossing and root coordinates in events. The rounding errors introduced (and clipping in core/XI 1.x events) can lead to the actual position being different to the requested input coordinates. e.g. 200 scales to 199.9999, truncated to 199 in the event. Avoid this by simply overwriting the scaled screen coordinates with the input coordinates for the POINTER_SCREEN case. X.Org Bug 53037 Signed-off-by: Peter Hutterer Reviewed-by: Keith Packard --- dix/getevents.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/dix/getevents.c b/dix/getevents.c index b3bb162ae..4e62507aa 100644 --- a/dix/getevents.c +++ b/dix/getevents.c @@ -1327,6 +1327,7 @@ fill_pointer_events(InternalEvent *events, DeviceIntPtr pDev, int type, RawDeviceEvent *raw; double screenx = 0.0, screeny = 0.0; /* desktop coordinate system */ double devx = 0.0, devy = 0.0; /* desktop-wide in device coords */ + int sx, sy; /* for POINTER_SCREEN */ ValuatorMask mask; ScreenPtr scr; @@ -1369,8 +1370,11 @@ fill_pointer_events(InternalEvent *events, DeviceIntPtr pDev, int type, /* valuators are in driver-native format (rel or abs) */ if (flags & POINTER_ABSOLUTE) { - if (flags & POINTER_SCREEN) /* valuators are in screen coords */ + if (flags & POINTER_SCREEN) { /* valuators are in screen coords */ + sx = valuator_mask_get(&mask, 0); + sy = valuator_mask_get(&mask, 1); scale_from_screen(pDev, &mask); + } transformAbsolute(pDev, &mask); clipAbsolute(pDev, &mask); @@ -1388,6 +1392,18 @@ fill_pointer_events(InternalEvent *events, DeviceIntPtr pDev, int type, /* valuators are in device coordinate system in absolute coordinates */ scale_to_desktop(pDev, &mask, &devx, &devy, &screenx, &screeny); + + /* #53037 XWarpPointer's scaling back and forth between screen and + device may leave us with rounding errors. End result is that the + pointer doesn't end up on the pixel it should. + Avoid this by forcing screenx/screeny back to what the input + coordinates were. + */ + if (flags & POINTER_SCREEN) { + screenx = sx; + screeny = sy; + } + scr = positionSprite(pDev, (flags & POINTER_ABSOLUTE) ? Absolute : Relative, &mask, &devx, &devy, &screenx, &screeny); From d53e6e02a2595ced1882f5fcd34d08ea039b3b85 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 16 Aug 2012 13:54:42 +1000 Subject: [PATCH 7/8] mi: don't check for core events in miPointerSetPosition (#53568) As of 81cfe44b1ed0de84ad1941fe2ca74bebef3fc58d, miPointerSetPosition now returns the screen pointer of the device. This broke floating slave devices, as soon as a motion event was submitted, miPointerSetPosition returned NULL, crashing the server. dev->coreEvents is only false if the device is a floating slave, in which case it has a sprite. X.Org Bug 53568 Signed-off-by: Peter Hutterer Reviewed-by: Keith Packard --- mi/mipointer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mi/mipointer.c b/mi/mipointer.c index a56838ead..4defaf5ec 100644 --- a/mi/mipointer.c +++ b/mi/mipointer.c @@ -575,7 +575,7 @@ miPointerSetPosition(DeviceIntPtr pDev, int mode, double *screenx, miPointerPtr pPointer; - if (!pDev || !pDev->coreEvents) + if (!pDev) return NULL; pPointer = MIPOINTER(pDev); From 24ffcfcded6b4b024958801e8a6cecad36d9a3e3 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 20 Aug 2012 10:28:26 +1000 Subject: [PATCH 8/8] os: fix typo, fsync when WIN32 is _not_ defined Introduced in 164b38c72fe9c69d13ea4f9c46d4ccc46566d826 Reported-by: Jon TURNEY Signed-off-by: Peter Hutterer Reviewed-by: Keith Packard --- os/log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/os/log.c b/os/log.c index a0628fda3..4820e9a77 100644 --- a/os/log.c +++ b/os/log.c @@ -374,7 +374,7 @@ LogSWrite(int verb, const char *buf, size_t len, Bool end_line) if (verb < 0 || logFileVerbosity >= verb) { if (inSignalContext && logFileFd >= 0) { write(logFileFd, buf, len); -#ifdef WIN32 +#ifndef WIN32 if (logFlush && logSync) fsync(logFileFd); #endif