[PATCH v3 0/7] trace2: stop allowing die()

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

NOTE: this v3 is rebased onto a recent 'master' due to conflicts in a test
script.

After v1 was posted, based on a concrete example of tracing leading to a
recursive die() problem, more evidence has come up to imply that allocations
are failing for some users more often. This is potentially an issue with the
allocator chosen by Git for Windows, which is being discussed elsewhere.

But the conclusion is this: the trace2 API shouldn't call helpers that might
call die(). It's too low-level for that.

In this v2, I have a much more robust approach to removing die() from the
trace2 API.

This starts with a new banned-die.h header file at the root of the repo and
including it from all trace2 API *.c files. It starts empty, but the later
patches will add one method at a time:

 * xsnprintf() : This is the original patch, but made more complete by
   adding the method to banned-die.h.
 * xstrdup()
 * ALLOC_ARRAY()
 * xstrfmt()
 * ALLOC_GROW()
 * xcalloc()

During each patch, the goal was to have the trace2 logic be "as correct as
possible" when an allocation failure occurs. This may mean that we have
incomplete messages or dropped trace messages.

The focus here is that the trace2 API should never cause a process-ending
failure, because those failures will trigger trace2 API calls while
reporting the failure.


Updates in V3
=============

 * Peff correctly points out that this is far from complete, as the strbuf
   library is not safe from die(). The banned-die.h provides incremental
   demonstration that these changes are showing progress and preventing
   regression in future changes, but not showing a complete picture. I will
   start an investigation into a "safe" or "gentle" variant of the strbuf
   API as a potential direction for these API layers.
 * The first patch had a lowercase banned() that should have been uppercase
   BANNED().
 * A 'return -1' was replaced with 'return 0' to avoid a misleading error
   message.
 * The ":<REDACTED>" string length was incorrect. This is fixed and tests
   are improved to cover this string manipulation. These test changes
   conflict with changes to use test_grep in 47f79f61983 (t: convert grep
   assertions to test_grep, 2026-07-06), so this v3 is rebased onto
   'master'.
 * Patch 6 was previously failing at runtime. The appropriate fix is pulled
   out of patch 7 and into patch 6.

Thanks, -Stolee

Derrick Stolee (7):
  banned-die: create header for banning of functions
  trace2: tolerate failed timestamp formatting
  trace2: remove use of xstrdup()
  trace2: remove use of ALLOC_ARRAY()
  trace2: remove use of xstrfmt()
  trace2: remove use of ALLOC_GROW()
  trace2: remove use of xcalloc()

 banned-die.h            | 32 +++++++++++++++++
 t/t0212-trace2-event.sh | 12 ++++---
 trace2.c                | 52 +++++++++++++++++++++++++---
 trace2/tr2_cfg.c        |  2 ++
 trace2/tr2_cmd_name.c   |  2 ++
 trace2/tr2_ctr.c        | 12 ++++++-
 trace2/tr2_dst.c        |  2 ++
 trace2/tr2_sid.c        |  2 ++
 trace2/tr2_sysenv.c     |  8 +++--
 trace2/tr2_tbuf.c       | 51 +++++++++++++++++++--------
 trace2/tr2_tgt_event.c  |  2 ++
 trace2/tr2_tgt_normal.c |  2 ++
 trace2/tr2_tgt_perf.c   |  2 ++
 trace2/tr2_tls.c        | 77 +++++++++++++++++++++++++++++++++++++++--
 trace2/tr2_tls.h        |  7 ++++
 trace2/tr2_tmr.c        | 16 +++++++--
 16 files changed, 250 insertions(+), 31 deletions(-)
 create mode 100644 banned-die.h


base-commit: c73e85354c275c9d409b26445089bc16940fc527
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2178

Range-diff vs v2:

 1:  84634717e2 ! 1:  c483a4bf76 banned-die: create header for banning of functions
     @@ banned-die.h (new)
      + */
      +
      +#undef die
     -+#define die banned(die)
     ++#define die BANNED(die)
      +
      +#endif /* BANNED_DIE_H */
      
     @@ trace2.c
       #include "trace2/tr2_tgt.h"
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_tmr.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       static int trace2_enabled;
     @@ trace2/tr2_cfg.c
       #include "trace2/tr2_cfg.h"
       #include "trace2/tr2_sysenv.h"
       #include "wildmatch.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;
     @@ trace2/tr2_cmd_name.c
       #include "git-compat-util.h"
       #include "strbuf.h"
       #include "trace2/tr2_cmd_name.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       #define TR2_ENVVAR_PARENT_NAME "GIT_TRACE2_PARENT_NAME"
     @@ trace2/tr2_ctr.c
       #include "trace2/tr2_tgt.h"
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_ctr.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       /*
     @@ trace2/tr2_dst.c
       #include "trace2/tr2_dst.h"
       #include "trace2/tr2_sid.h"
       #include "trace2/tr2_sysenv.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       /*
     @@ trace2/tr2_sid.c
       #include "strbuf.h"
       #include "trace2/tr2_tbuf.h"
       #include "trace2/tr2_sid.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       #define TR2_ENVVAR_PARENT_SID "GIT_TRACE2_PARENT_SID"
     @@ trace2/tr2_sysenv.c
       #include "config.h"
       #include "dir.h"
       #include "tr2_sysenv.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       /*
     @@ trace2/tr2_tbuf.c
      @@
       #include "git-compat-util.h"
       #include "tr2_tbuf.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       void tr2_tbuf_local_time(struct tr2_tbuf *tb)
     @@ trace2/tr2_tgt_event.c
       #include "trace2/tr2_tgt.h"
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_tmr.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       static struct tr2_dst tr2dst_event = {
     @@ trace2/tr2_tgt_normal.c
       #include "trace2/tr2_tgt.h"
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_tmr.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       static struct tr2_dst tr2dst_normal = {
     @@ trace2/tr2_tgt_perf.c
       #include "trace2/tr2_tgt.h"
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_tmr.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       static struct tr2_dst tr2dst_perf = {
     @@ trace2/tr2_tls.c
       #include "thread-utils.h"
       #include "trace.h"
       #include "trace2/tr2_tls.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       /*
     @@ trace2/tr2_tmr.c
       #include "trace2/tr2_tls.h"
       #include "trace2/tr2_tmr.h"
       #include "trace.h"
     ++/* banned-die must be last. */
      +#include "banned-die.h"
       
       #define MY_MAX(a, b) ((a) > (b) ? (a) : (b))
 2:  bd45f46a34 ! 2:  754fffb74e trace2: tolerate failed timestamp formatting
     @@ Commit message
       ## banned-die.h ##
      @@
       #undef die
     - #define die banned(die)
     + #define die BANNED(die)
       
      +#undef xsnprintf
      +#define xsnprintf(...) BANNED(xsnprintf)
 3:  ec447a6a77 ! 3:  87d3f1b557 trace2: remove use of xstrdup()
     @@ Commit message
          trace2/tr2_sysenv.c.
      
          First, in tr2_sysenv_cb(), we need to handle a failed assignment of the
     -    value with a negative return to halt the config parsing loop.
     +    value with a zero-valued return to halt the config parsing loop. Note
     +    that we don't want to use a negative return here or we would imply to
     +    the config system that the config key or value was somehow invalid; such
     +    an output would mask the real issue that the process failed to allocate
     +    memory.
      
          Second, in tr2_sysenv_get(), the method will return NULL when strdup()
          returns NULL. This return is indistinguishable from the environment variable
     @@ Commit message
          failure at this level will likely lead to failure in another system, but at
          least the trace2 API will not cause the process to fail early.
      
     +    Helped-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
       ## banned-die.h ##
     @@ trace2/tr2_sysenv.c: static int tr2_sysenv_cb(const char *key, const char *value
      -			tr2_sysenv_settings[k].value = xstrdup(value);
      +			tr2_sysenv_settings[k].value = strdup(value);
      +			if (!tr2_sysenv_settings[k].value)
     -+				return -1;
     ++				return 0;
       			return 0;
       		}
       	}
 4:  db6858d381 = 4:  5bf6ab91f3 trace2: remove use of ALLOC_ARRAY()
 5:  7f0bb405ad ! 5:  3e419c5522 trace2: remove use of xstrfmt()
     @@ Commit message
          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 ##
     @@ banned-die.h
       #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)
       
      
     + ## t/t0212-trace2-event.sh ##
     +@@ t/t0212-trace2-event.sh: 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' '
     +@@ t/t0212-trace2-event.sh: 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' '
     +@@ t/t0212-trace2-event.sh: 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' '
     +@@ t/t0212-trace2-event.sh: 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
     +
       ## trace2.c ##
      @@ trace2.c: int trace2_is_enabled(void)
       static const char *redact_arg(const char *arg)
     @@ trace2.c: static const char *redact_arg(const char *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) ||
     ++	    unsigned_add_overflows(prefix_len + suffix_len + redact_len, 1))
      +		return NULL;
      +
     -+	redacted_len = prefix_len + suffix_len + redact_len;
     ++	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 - 1);
     -+	memcpy(redacted + prefix_len + redact_len - 1, p + at,
     -+	       suffix_len + 1);
     ++	memcpy(redacted + prefix_len, redact, redact_len);
     ++	memcpy(redacted + prefix_len + redact_len, p + at, suffix_len + 1);
      +	return redacted;
       }
       
 6:  120cf1967b ! 6:  ccd284fbeb trace2: remove use of ALLOC_GROW()
     @@ Commit message
          deepening the stack, giving as much nesting behavior as possible without
          failing the entire process.
      
     +    Helped-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
       ## banned-die.h ##
     @@ trace2/tr2_tls.c: void tr2tls_unset_self(void)
      +		return;
      +	}
      +
     -+	if (ctx->nr_open_regions < ctx->alloc)
     -+		return;
     ++	if (ctx->nr_open_regions >= ctx->alloc) {
     ++		if (ctx->alloc >
     ++		    SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {
     ++			ctx->nr_skipped_regions++;
     ++			return;
     ++		}
     ++		new_alloc = ctx->alloc * 2;
      +
     -+	if (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {
     -+		ctx->nr_skipped_regions++;
     -+		return;
     -+	}
     -+	new_alloc = ctx->alloc * 2;
     ++		new_array = realloc(ctx->array_us_start,
     ++				    new_alloc * sizeof(*ctx->array_us_start));
     ++		if (!new_array) {
     ++			ctx->nr_skipped_regions++;
     ++			return;
     ++		}
      +
     -+	new_array = realloc(ctx->array_us_start,
     -+			    new_alloc * sizeof(*ctx->array_us_start));
     -+	if (!new_array) {
     -+		ctx->nr_skipped_regions++;
     -+		return;
     ++		ctx->array_us_start = new_array;
     ++		ctx->alloc = new_alloc;
      +	}
     -+
     -+	ctx->array_us_start = new_array;
     -+	ctx->alloc = new_alloc;
       
      -	ALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc);
       	ctx->array_us_start[ctx->nr_open_regions++] = us_now;
 7:  c8fc195a2a ! 7:  fa10e8d246 trace2: remove use of xcalloc()
     @@ trace2/tr2_tls.c: void tr2tls_push_self(uint64_t us_now)
       	uint64_t *new_array;
       	size_t new_alloc;
       
     --	if (ctx->nr_skipped_regions) {
     --		ctx->nr_skipped_regions++;
     --		return;
     --	}
     --
     --	if (ctx->nr_open_regions < ctx->alloc)
      +	if (tr2tls_is_fallback(ctx))
     - 		return;
     - 
     --	if (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {
     -+	if (ctx->nr_skipped_regions) {
     ++		return;
     ++
     + 	if (ctx->nr_skipped_regions) {
       		ctx->nr_skipped_regions++;
       		return;
     - 	}
     --	new_alloc = ctx->alloc * 2;
     - 
     --	new_array = realloc(ctx->array_us_start,
     --			    new_alloc * sizeof(*ctx->array_us_start));
     --	if (!new_array) {
     --		ctx->nr_skipped_regions++;
     --		return;
     -+	if (ctx->nr_open_regions >= ctx->alloc) {
     -+		if (ctx->alloc >
     -+		    SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {
     -+			ctx->nr_skipped_regions++;
     -+			return;
     -+		}
     -+		new_alloc = ctx->alloc * 2;
     -+
     -+		new_array = realloc(ctx->array_us_start,
     -+				    new_alloc * sizeof(*ctx->array_us_start));
     -+		if (!new_array) {
     -+			ctx->nr_skipped_regions++;
     -+			return;
     -+		}
     -+
     -+		ctx->array_us_start = new_array;
     -+		ctx->alloc = new_alloc;
     - 	}
     - 
     --	ctx->array_us_start = new_array;
     --	ctx->alloc = new_alloc;
     --
     - 	ctx->array_us_start[ctx->nr_open_regions++] = us_now;
     - }
     - 
      @@ trace2/tr2_tls.c: void tr2tls_pop_self(void)
       {
       	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();

-- 
gitgitgadget