commit 511822529ae1710e141bc26199ec5ff1d1abcd16 Author: Nick Mathewson <nickm@torproject.org> Date: Wed Oct 28 09:39:21 2020 -0400 Revise fix for bug 32178 (spaces at end of log msg). The loop in the earlier patch would invoke undefined behavior in two ways: First, it would check whether it was looking at a space before it checked whether the pointer was in-range. Second, it would let a pointer reach a position _before_ the start of a string, which is not allowed. I've removed the assertion about empty messages: empty messages can be their own warning IMO. I've also added tests for this formatting code, to make sure it actually works. --- changes/ticket32178 | 2 +- src/feature/control/control_events.c | 36 ++++++++++++++++++++++-------------- src/feature/control/control_events.h | 2 ++ src/test/test_controller_events.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/changes/ticket32178 b/changes/ticket32178 index ae8554d8a0..c13e490cb0 100644 --- a/changes/ticket32178 +++ b/changes/ticket32178 @@ -1,3 +1,3 @@ o Minor bugfixes (logging): - Remove trailing whitespaces from control event log messages. Fixes bug - 32178. + 32178; bugfix on 0.1.1.1-alpha. Based on a patch by Amadeusz Pawlik. diff --git a/src/feature/control/control_events.c b/src/feature/control/control_events.c index 57cfb0d026..0dd52659ec 100644 --- a/src/feature/control/control_events.c +++ b/src/feature/control/control_events.c @@ -1352,6 +1352,27 @@ enable_control_logging(void) tor_assert(0); } +/** Remove newline and carriage-return characters from @a msg, replacing them + * with spaces, and discarding any that appear at the end of the message */ +void +control_logmsg_strip_newlines(char *msg) +{ + char *cp; + for (cp = msg; *cp; ++cp) { + if (*cp == '\r' || *cp == '\n') { + *cp = ' '; + } + } + if (cp == msg) + return; + /* Remove trailing spaces */ + for (--cp; *cp == ' '; --cp) { + *cp = '\0'; + if (cp == msg) + break; + } +} + /** We got a log message: tell any interested control connections. */ void control_event_logmsg(int severity, log_domain_mask_t domain, const char *msg) @@ -1380,21 +1401,8 @@ control_event_logmsg(int severity, log_domain_mask_t domain, const char *msg) char *b = NULL; const char *s; if (strchr(msg, '\n')) { - char *cp; b = tor_strdup(msg); - for (cp = b; *cp; ++cp) - if (*cp == '\r' || *cp == '\n') - *cp = ' '; - - /* Remove trailing spaces */ - for (--cp; *cp == ' ' && cp >= b; --cp) - *cp = '\0'; - - if ( cp == b ){ - ++disable_log_messages; - tor_assert_nonfatal(*b); - --disable_log_messages; - } + control_logmsg_strip_newlines(b); } switch (severity) { case LOG_DEBUG: s = "DEBUG"; break; diff --git a/src/feature/control/control_events.h b/src/feature/control/control_events.h index 6e3cfef4e9..0ac233cc6e 100644 --- a/src/feature/control/control_events.h +++ b/src/feature/control/control_events.h @@ -341,6 +341,8 @@ struct control_event_t { extern const struct control_event_t control_event_table[]; +void control_logmsg_strip_newlines(char *msg); + #ifdef TOR_UNIT_TESTS MOCK_DECL(STATIC void, send_control_event_string,(uint16_t event, const char *msg)); diff --git a/src/test/test_controller_events.c b/src/test/test_controller_events.c index 60dfbd630a..3cd529fa10 100644 --- a/src/test/test_controller_events.c +++ b/src/test/test_controller_events.c @@ -436,6 +436,33 @@ test_cntev_signal(void *arg) UNMOCK(queue_control_event_string); } +static void +test_cntev_log_fmt(void *arg) +{ + (void) arg; + char *result = NULL; +#define CHECK(pre, post) \ + do { \ + result = tor_strdup((pre)); \ + control_logmsg_strip_newlines(result); \ + tt_str_op(result, OP_EQ, (post)); \ + tor_free(result); \ + } while (0) + + CHECK("There is a ", "There is a"); + CHECK("hello", "hello"); + CHECK("", ""); + CHECK("Put spaces at the end ", "Put spaces at the end"); + CHECK(" ", ""); + CHECK("\n\n\n", ""); + CHECK("Testing\r\n", "Testing"); + CHECK("T e s t\ni n g\n", "T e s t i n g"); + + done: + tor_free(result); +#undef CHECK +} + static void setup_orconn_state(orconn_state_msg_t *msg, uint64_t gid, uint64_t chan, int proxy_type) @@ -718,6 +745,7 @@ struct testcase_t controller_event_tests[] = { TEST(event_mask, TT_FORK), TEST(format_stream, TT_FORK), TEST(signal, TT_FORK), + TEST(log_fmt, 0), T_PUBSUB(dirboot_defer_desc, TT_FORK), T_PUBSUB(dirboot_defer_orconn, TT_FORK), T_PUBSUB(orconn_state, TT_FORK),