[PATCH v3 5/7] trace2: remove use of xstrfmt()

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

From: Derrick Stolee <stolee@gmail.com>

We continue removing the possibility of a die() in the trace2 API by
banning xstrfmt(), which calls die() during a failure to format. Instead
of allowing a die(), perform a soft failure by failing to output the
trace2 data when such a failure occurs.

This requires carefully concatenating strings using memcpy() to
construct redacted data to avoid copying password information in traced
URLs.

Update t0212 to more carefully test this behavior to explicitly include
the ":<REDACTED>" string in the appropriate context.

Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h            |  3 +++
 t/t0212-trace2-event.sh | 12 ++++++++----
 trace2.c                | 34 ++++++++++++++++++++++++++++++++--
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index 0ad9a6c492..4d1800353d 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -17,6 +17,9 @@
 #undef xstrdup
 #define xstrdup(str) BANNED(xstrdup)
 
+#undef xstrfmt
+#define xstrfmt(...) BANNED(xstrfmt)
+
 #undef ALLOC_ARRAY
 #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)
 
diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh
index f5358a1dd4..23df800395 100755
--- a/t/t0212-trace2-event.sh
+++ b/t/t0212-trace2-event.sh
@@ -332,7 +332,8 @@ test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
 		test-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+	test_grep ! user:pwd trace.event &&
+	test_grep "user:<REDACTED>@example.com/" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in child_start events' '
@@ -341,7 +342,8 @@ test_expect_success 'unsafe URLs are redacted by default in child_start events'
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
 		test-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+	test_grep ! user:pwd trace.event &&
+	test_grep "user:<REDACTED>@example.com/" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in exec events' '
@@ -350,7 +352,8 @@ test_expect_success 'unsafe URLs are redacted by default in exec events' '
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
 		test-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&
-	test_grep ! user:pwd trace.event
+	test_grep ! user:pwd trace.event &&
+	test_grep "user:<REDACTED>@example.com/" trace.event
 '
 
 test_expect_success 'unsafe URLs are redacted by default in def_param events' '
@@ -359,7 +362,8 @@ test_expect_success 'unsafe URLs are redacted by default in def_param events' '
 
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
 		test-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&
-	test_grep ! user:pwd trace.event
+	test_grep ! user:pwd trace.event &&
+	test_grep "user:<REDACTED>@example.com/" trace.event
 '
 
 test_done
diff --git a/trace2.c b/trace2.c
index ea021c602e..4a597d8213 100644
--- a/trace2.c
+++ b/trace2.c
@@ -261,7 +261,10 @@ int trace2_is_enabled(void)
 static const char *redact_arg(const char *arg)
 {
 	const char *p, *colon;
+	const char *redact = ":<REDACTED>";
+	char *redacted;
 	size_t at;
+	size_t prefix_len, suffix_len, redacted_len, redact_len;
 
 	if (!trace2_redact ||
 	    (!skip_prefix(arg, "https://", &p) &&
@@ -276,7 +279,25 @@ static const char *redact_arg(const char *arg)
 	if (!colon)
 		return arg;
 
-	return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
+	redact_len = strlen(redact);
+	prefix_len = colon - arg;
+	suffix_len = strlen(p + at);
+
+	if (unsigned_add_overflows(prefix_len, suffix_len) ||
+	    unsigned_add_overflows(prefix_len + suffix_len, redact_len) ||
+	    unsigned_add_overflows(prefix_len + suffix_len + redact_len, 1))
+		return NULL;
+
+	redacted_len = prefix_len + suffix_len + redact_len + 1;
+
+	redacted = malloc(redacted_len);
+	if (!redacted)
+		return NULL;
+
+	memcpy(redacted, arg, prefix_len);
+	memcpy(redacted + prefix_len, redact, redact_len);
+	memcpy(redacted + prefix_len + redact_len, p + at, suffix_len + 1);
+	return redacted;
 }
 
 /*
@@ -301,6 +322,8 @@ static const char **redact_argv(const char **argv)
 
 	if (!argv[i])
 		return argv;
+	if (!redacted)
+		return NULL;
 
 	for (j = 0; argv[j]; j++)
 		; /* keep counting */
@@ -317,7 +340,14 @@ static const char **redact_argv(const char **argv)
 	ret[i] = redacted;
 	for (++i; argv[i]; i++) {
 		redacted = redact_arg(argv[i]);
-		ret[i] = redacted ? redacted : argv[i];
+		if (!redacted) {
+			for (j = 0; j < i; j++)
+				if (ret[j] != argv[j])
+					free((void *)ret[j]);
+			free(ret);
+			return NULL;
+		}
+		ret[i] = redacted;
 	}
 
 	return ret;
-- 
gitgitgadget