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.

3 files changed, 52 insertions, 14 deletions

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
@@ -437,6 +437,33 @@ test_cntev_signal(void *arg)
 }
 
 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),