From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Thu, 2 May 2024 07:30:28 +0000 (+0200)
Subject: BUG/MINOR: log: prevent double spaces emission in sess_build_logline()
X-Git-Tag: v3.0-dev10~5
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cc2e94a9480557afb86ea8777bac49196d6f1715;p=thirdparty%2Fhaproxy.git

BUG/MINOR: log: prevent double spaces emission in sess_build_logline()

Christian reported in GH #2556 that since 3.0-dev double spaces may be
found in log messages on some cases where it was not the case before.

As we were able to easily reproduce, a quick bisect led us to c6a7138
("MINOR: log: simplify last_isspace in sess_build_logline()"). While
it is true that all switch cases set the last_isspace variable to 0,
there was a subtelty for some fields such as '%hr', '%hrl', '%hs' or
'%hsl' and I overlooked it. Indeed, for '%hr', last_isspace was only set
to 0 if data was emitted, else the assignment didn't occur.

But with c6a7138, last_isspace is always set to 0 as long as the current
node type is not a separator. Because of that, if no data is emitted for
the current node value, and a space was already emitted prior to the
current node, then an extra space could be emitted after the node,
resulting in two spaces being emitted.

Note that while c6a7138 introduces a slight behavior regression regarding
last_isspace logic with the specific fields mentionned above, this
behavior could already be triggered with a failing or empty logformat
node sample expression. Consider this logformat expression:

  log-format "%{-M}o | %[str()] |"

str() will not print anything, and since we disabled mandatory option with
'-M', nothing gets printed for the node sample expression. As a result, we
have the following output:

  "|  |"

Instead of (when mandatory option is enabled):

  "| - |"

Thus in order to stick to the historical behavior, systematically set
last_isspace to 0 for EXPR nodes, and only set last_isspace to 0 when
data was written for TAG nodes. This way, '%hr', '%hrl', '%hs' or
'%hsl' should behave as before.

No backport needed.
---

diff --git a/src/log.c b/src/log.c
index f6ab921c20..14c03cc18d 100644
--- a/src/log.c
+++ b/src/log.c
@@ -3797,6 +3797,7 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 			if (ret == NULL)
 				goto out;
 			tmplog = ret;
+			last_isspace = 0; /* consider that data was written */
 			goto next_fmt;
 		}
 
@@ -4756,8 +4757,6 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 
 		}
  next_fmt:
-		last_isspace = 0;
-
 		if (value_beg == tmplog) {
 			/* handle the case where no data was generated for the value after
 			 * the key was already announced
@@ -4782,6 +4781,13 @@ int sess_build_logline(struct session *sess, struct stream *s, char *dst, size_t
 		 * to end it
 		 */
 		LOG_VARTEXT_END();
+		if (tmplog != value_beg) {
+			/* data was actually generated for the current dynamic
+			 * node, reset the space hint so that a new space may
+			 * now be emitted when relevant.
+			 */
+			last_isspace = 0;
+		}
 	}
 
 	/* back to global ctx (some encoding types may need to output