From 511822529ae1710e141bc26199ec5ff1d1abcd16 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 28 Oct 2020 09:39:21 -0400
Subject: [PATCH] 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),
--
GitLab