[PATCH] trace2: tolerate failed timestamp formatting

[PATCH] trace2: tolerate failed timestamp formatting

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

Some users reported issues of repeated messages:

  fatal: recursion detected in die handler

This wasn't happening every time, but we eventually captured a
GIT_TRACE2_PERF log file with this issue and revealed an interesting
internal detail, failing with this message:

  unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ

This specific format string tracks to tr2_tbuf_utc_datetime_extended()
in trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in
ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but
was later split in bad229aef23 (trace2: clarify UTC datetime formatting,
2019-04-15).

This use of xsnprintf() is writing a very specific datetime format into a
32-character buffer. The format requires that the input data will not
overflow the format digits or the buffer will not hold the result. Since
we are using xsnprintf() here, those failures turn into die() events.

This method and its siblings, tr2_tbuf_local_time() and
tr2_tbuf_utc_datetime(), are used in the tracing library. The extended
form is used only for the 'event' format, which these users were using
via a config setting for use in client-side telemetry. The non-extended
form is used to help generate the 'SID' that defines the process in the
traces.

Not only are these inappropriate times for a failure, but the extended
method is called specifially during the 'atexit' event, which was
triggering this problem in a loop as the 'atexit' event would be
retriggered by the die().

I could not determine the exact cause of why these errors started
occuring in a bunch. My best guess is that these users are dogfooding an
early operating system version that is more likely to fail in the
gettimeofday() function and thus leaves the structures uninitialized and
potentially violating the expected values.

However, for full defense-in-depth I made several modifications:

1. Both 'tv' and 'tm' structs are initialized with zero values, allowing
   an erroring gettimeofday() or gmtime_r() method to leave them
   zero-valued. A zero-valued date is better than a die() here.

2. Replace the use of xsnprintf() with snprintf() to avoid the
   possibility of calling die() here. Instead, check the response to see
   if there was a failure. On failure, put a blank value into the buffer
   instead of possibly allowing a value that would not format correctly
   for a trace2 consumer. This value should be seen as obviously wrong
   and therefore signals a problem.

As the core issue in this code seems to require a system method
returning an error, no test accompanies this change.

This change removes all uses of xsnprintf() from the trace2/ directory.
There are two uses of xstrdup() that could be considered for removal,
but they only die() on out-of-memory errors instead of formatting
issues. I chose to leave those in place for now.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
    trace2: tolerate failed timestamp formatting
    
    As mentioned, this is based on real trace logs of failed commands users
    are seeing.
    
    I wish I had a better way to test this or to be 100% sure that the
    system call was failing. But users were seeing failures and these seemed
    like appropriate changes.
    
    Thanks, -Stolee

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

 trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c
index c3b3822ed7..ef57376f3c 100644
--- a/trace2/tr2_tbuf.c
+++ b/trace2/tr2_tbuf.c
@@ -3,45 +3,64 @@
 
 void tr2_tbuf_local_time(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	localtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld", tm.tm_hour,
-		  tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld",
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "00:00:00.000000";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }
 
 void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	gmtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf),
-		  "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ", tm.tm_year + 1900,
-		  tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,
-		  (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf),
+		       "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ",
+		       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "1900-00-00T00:00:00.000000Z";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }
 
 void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	gmtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf), "%4d%02d%02dT%02d%02d%02d.%06ldZ",
-		  tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
-		  tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf),
+		       "%4d%02d%02dT%02d%02d%02d.%06ldZ",
+		       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "19000000T000000.000000Z";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
gitgitgadget

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Taylor Blau]

On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:
> This change removes all uses of xsnprintf() from the trace2/ directory.
> There are two uses of xstrdup() that could be considered for removal,
> but they only die() on out-of-memory errors instead of formatting
> issues. I chose to leave those in place for now.

I may be missing some Git for Windows context, but I dug into this a
little and I'm not sure 'gettimeofday()' is the culprit...

In my understanding Git for Windows's 'gettext.h' appears[1] to redirect
the 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this
case, we have seven '%' placeholders. Gettext can store only six plus
its end marker inline, so parsing the seventh causes an allocation
before any timestamp values are read.

A failure there would produce the observed -1, after which 'xsnprintf()'
dies and trace2 can recurse.

I think that also explains why calling 'snprintf()' directly helps.
tr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If
I'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and
always returns zero [2], making the zero-initialization unrelated.

Would it make more sense to fix the xsnprintf()/libintl boundary and
treat Trace2 reentrancy separately? I still can't explain why the
allocation failed, so there may be another GfW-specific piece I’m
missing.

I think something like the following (untested) would prevent the
redirection to `libintl_vsnprintf()`:

--- 8< ---
diff --git a/wrapper.c b/wrapper.c
index 16f5a63fbb..2976d4e110 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -7,7 +7,14 @@
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "parse.h"
+
+/*
+ * xsnprintf() only formats non-translated strings. On MinGW, avoid
+ * redirecting its vsnprintf() call to libintl's allocating replacement.
+ */
+#define _INTL_NO_DEFINE_MACRO_VSNPRINTF
 #include "gettext.h"
+#undef _INTL_NO_DEFINE_MACRO_VSNPRINTF
 #include "strbuf.h"
 #include "trace2.h"
--- >8 ---

Thanks,
Taylor

[1]: https://github.com/git-for-windows/git-sdk-64/blob/1351ad2fc39a1f74c56b2cc2b38107ec8df8eb40/mingw64/include/libintl.h#L731-L754
[2]: https://github.com/microsoft/git/blob/vfs-2.55.0/compat/mingw.c#L1609-L1618

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Derrick Stolee]

On 7/17/2026 12:24 PM, Taylor Blau wrote:
> On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:
>> This change removes all uses of xsnprintf() from the trace2/ directory.
>> There are two uses of xstrdup() that could be considered for removal,
>> but they only die() on out-of-memory errors instead of formatting
>> issues. I chose to leave those in place for now.
> 
> I may be missing some Git for Windows context, but I dug into this a
> little and I'm not sure 'gettimeofday()' is the culprit...
> 
> In my understanding Git for Windows's 'gettext.h' appears[1] to redirect
> the 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this
> case, we have seven '%' placeholders. Gettext can store only six plus
> its end marker inline, so parsing the seventh causes an allocation
> before any timestamp values are read.
> 
> A failure there would produce the observed -1, after which 'xsnprintf()'
> dies and trace2 can recurse.

With this perspective, the issue is that gettext is doing dynamic
allocation and getting a failure there, which explains the transient
nature. This is an interesting idea, and a more likely "application
side" error. I'm still curious why this is creeping up for the first
time in this burst, since nothing has changed in the application, to
my knowledge. 
> I think that also explains why calling 'snprintf()' directly helps.
> tr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If
> I'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and
> always returns zero [2], making the zero-initialization unrelated.
> 
> Would it make more sense to fix the xsnprintf()/libintl boundary and
> treat Trace2 reentrancy separately? I still can't explain why the
> allocation failed, so there may be another GfW-specific piece I’m
> missing.

I think that your suggested change has merits and should be pursued.
I'll explore it a bit to confirm.

The other justification I'd like to make in my patch is that the
xsnprintf() calls die() and the trace2 machinery should be die()-free
whenever possible. Solving both possible causes is likely the right
long-term approach.

Thanks,
-Stolee

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>> Would it make more sense to fix the xsnprintf()/libintl boundary and
>> treat Trace2 reentrancy separately? I still can't explain why the
>> allocation failed, so there may be another GfW-specific piece I’m
>> missing.
>
> I think that your suggested change has merits and should be pursued.
> I'll explore it a bit to confirm.

That band-aid may be a good idea, but I would prefer not to see the
conditional in a common source file like 'wrapper.c'.  Somewhere
MinGW-specific would be more appropriate, would it not?

> The other justification I'd like to make in my patch is that the
> xsnprintf() calls die() and the trace2 machinery should be die()-free
> whenever possible. Solving both possible causes is likely the right
> long-term approach.

That is indeed worth considering.

You mention a few calls to xstrdup() that can potentially abort, and
I agree that anything that triggers malloc() and notices that we are
out of memory can probably do little better than to die.  But are
there other operations that may cause us to exit, even though we are
not in an unrecoverable state (such as an out-of-memory condition)?

Thanks.

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Taylor Blau]

On Mon, Jul 20, 2026 at 07:29:51AM -0700, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
> >> Would it make more sense to fix the xsnprintf()/libintl boundary and
> >> treat Trace2 reentrancy separately? I still can't explain why the
> >> allocation failed, so there may be another GfW-specific piece I’m
> >> missing.
> >
> > I think that your suggested change has merits and should be pursued.
> > I'll explore it a bit to confirm.
>
> That band-aid may be a good idea, but I would prefer not to see the
> conditional in a common source file like 'wrapper.c'.  Somewhere
> MinGW-specific would be more appropriate, would it not?

Yeah, to be clear, I do not think that putting the '#define' here in
'wrapper.c' is appropriate, and included it in my original email only to
demonstrate the shape of the proposed solution.

Thanks,
Taylor

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Derrick Stolee <stolee@gmail.com> writes:
>
>>> Would it make more sense to fix the xsnprintf()/libintl boundary and
>>> treat Trace2 reentrancy separately? I still can't explain why the
>>> allocation failed, so there may be another GfW-specific piece I’m
>>> missing.
>>
>> I think that your suggested change has merits and should be pursued.
>> I'll explore it a bit to confirm.
>
> That band-aid may be a good idea, but I would prefer not to see the
> conditional in a common source file like 'wrapper.c'.  Somewhere
> MinGW-specific would be more appropriate, would it not?

Did anything come out of this discussion?

>
>> The other justification I'd like to make in my patch is that the
>> xsnprintf() calls die() and the trace2 machinery should be die()-free
>> whenever possible. Solving both possible causes is likely the right
>> long-term approach.
>
> That is indeed worth considering.
>
> You mention a few calls to xstrdup() that can potentially abort, and
> I agree that anything that triggers malloc() and notices that we are
> out of memory can probably do little better than to die.  But are
> there other operations that may cause us to exit, even though we are
> not in an unrecoverable state (such as an out-of-memory condition)?
>
> Thanks.

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Derrick Stolee]

On 7/29/2026 5:35 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>>> Would it make more sense to fix the xsnprintf()/libintl boundary and
>>>> treat Trace2 reentrancy separately? I still can't explain why the
>>>> allocation failed, so there may be another GfW-specific piece I’m
>>>> missing.
>>>
>>> I think that your suggested change has merits and should be pursued.
>>> I'll explore it a bit to confirm.
>>
>> That band-aid may be a good idea, but I would prefer not to see the
>> conditional in a common source file like 'wrapper.c'.  Somewhere
>> MinGW-specific would be more appropriate, would it not?
> 
> Did anything come out of this discussion?

Sorry that I've been unavailable to come back to this thread, but here
is what I've learned in the meantime:

* Taylor's hunch that the memory allocation is more likely at fault
  is seeming more and more correct. When we fixed this issue, other
  issues around memory allocation came to light.

* For that reason, I'll rework this patch to point at the allocation
  as the likely reason the parsing fails. Avoiding a die() in the
  tracing code is still critical.

* Thus, I'll also replace the xstrdup() in the trace code to avoid a
  die() due to allocation problems.

* I will take a deeper look at this wrapper change and how it might
  be done in a careful way, as Taylor says his patch was an example
  only and not the "right" way to do it.

Thanks,
-Stolee

Re: [PATCH] trace2: tolerate failed timestamp formatting

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> * Taylor's hunch that the memory allocation is more likely at fault
>   is seeming more and more correct. When we fixed this issue, other
>   issues around memory allocation came to light.
>
> * For that reason, I'll rework this patch to point at the allocation
>   as the likely reason the parsing fails. Avoiding a die() in the
>   tracing code is still critical.
>
> * Thus, I'll also replace the xstrdup() in the trace code to avoid a
>   die() due to allocation problems.
>
> * I will take a deeper look at this wrapper change and how it might
>   be done in a careful way, as Taylor says his patch was an example
>   only and not the "right" way to do it.

Thanks.

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

[Derrick Stolee via GitGitGadget]

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.

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 +++++++++++++++++
 trace2.c                | 51 ++++++++++++++++++++++++---
 trace2/tr2_cfg.c        |  1 +
 trace2/tr2_cmd_name.c   |  1 +
 trace2/tr2_ctr.c        | 11 +++++-
 trace2/tr2_dst.c        |  1 +
 trace2/tr2_sid.c        |  1 +
 trace2/tr2_sysenv.c     |  7 ++--
 trace2/tr2_tbuf.c       | 50 +++++++++++++++++++--------
 trace2/tr2_tgt_event.c  |  1 +
 trace2/tr2_tgt_normal.c |  1 +
 trace2/tr2_tgt_perf.c   |  1 +
 trace2/tr2_tls.c        | 76 +++++++++++++++++++++++++++++++++++++++--
 trace2/tr2_tls.h        |  7 ++++
 trace2/tr2_tmr.c        | 15 ++++++--
 15 files changed, 229 insertions(+), 27 deletions(-)
 create mode 100644 banned-die.h


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

Range-diff vs v1:

 -:  ---------- > 1:  84634717e2 banned-die: create header for banning of functions
 1:  95c546bb3b ! 2:  bd45f46a34 trace2: tolerate failed timestamp formatting
     @@ Commit message
          triggering this problem in a loop as the 'atexit' event would be
          retriggered by the die().
      
     -    I could not determine the exact cause of why these errors started
     -    occuring in a bunch. My best guess is that these users are dogfooding an
     -    early operating system version that is more likely to fail in the
     -    gettimeofday() function and thus leaves the structures uninitialized and
     -    potentially violating the expected values.
     +    Based on other symptoms impacting users on the version reporting these
     +    failures, it is most likely that this is actually a failure to allocate
     +    memory, which is a specific symptom in Git for Windows. That fork uses a
     +    different library for its implementation of vsprintf() which allocates
     +    an array when seven or more positional arguments exist in the formatting
     +    string, such as this one.
      
     -    However, for full defense-in-depth I made several modifications:
     +    Ultimately, the trace2 machinery is so low-level that it should not rely on
     +    any helper functions that perform error handling with die(), as that can
     +    trigger issues that would then be traced, causing this kind of recursive
     +    loop.
     +
     +    These changes help remove any use of die() within this file:
      
          1. Both 'tv' and 'tm' structs are initialized with zero values, allowing
             an erroring gettimeofday() or gmtime_r() method to leave them
     @@ Commit message
          but they only die() on out-of-memory errors instead of formatting
          issues. I chose to leave those in place for now.
      
     +    Helped-by: Taylor Blau <ttaylorr@openai.com>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
     + ## banned-die.h ##
     +@@
     + #undef die
     + #define die banned(die)
     + 
     ++#undef xsnprintf
     ++#define xsnprintf(...) BANNED(xsnprintf)
     ++
     + #endif /* BANNED_DIE_H */
     +
       ## trace2/tr2_tbuf.c ##
      @@
       
 -:  ---------- > 3:  ec447a6a77 trace2: remove use of xstrdup()
 -:  ---------- > 4:  db6858d381 trace2: remove use of ALLOC_ARRAY()
 -:  ---------- > 5:  7f0bb405ad trace2: remove use of xstrfmt()
 -:  ---------- > 6:  120cf1967b trace2: remove use of ALLOC_GROW()
 -:  ---------- > 7:  c8fc195a2a trace2: remove use of xcalloc()

-- 
gitgitgadget

[PATCH v2 1/7] banned-die: create header for banning of functions

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

We have universally-banned functions listed in banned.h since
c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of
the code should be more strict than others.

One such example is the trace2 API which runs during atexit() and can
prove to cause die()-handler recursion problems if it calls die().

Create a new banned-die.h header file that will ban some Git methods
that call die(). Include that in all trace2 API implementation files.
This currently only bans die() itself, and that was already not used.

It would be reasonable to name this file trace2/tr2_banned.h to be
specific to the trace2 API, but it seems like such a restriction would
be valuable to put in some other areas of the code, so adding it at the
root of the tree seems like a good long-term approach.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h            | 14 ++++++++++++++
 trace2.c                |  1 +
 trace2/tr2_cfg.c        |  1 +
 trace2/tr2_cmd_name.c   |  1 +
 trace2/tr2_ctr.c        |  1 +
 trace2/tr2_dst.c        |  1 +
 trace2/tr2_sid.c        |  1 +
 trace2/tr2_sysenv.c     |  1 +
 trace2/tr2_tbuf.c       |  1 +
 trace2/tr2_tgt_event.c  |  1 +
 trace2/tr2_tgt_normal.c |  1 +
 trace2/tr2_tgt_perf.c   |  1 +
 trace2/tr2_tls.c        |  1 +
 trace2/tr2_tmr.c        |  1 +
 14 files changed, 27 insertions(+)
 create mode 100644 banned-die.h

diff --git a/banned-die.h b/banned-die.h
new file mode 100644
index 0000000000..5eff361e55
--- /dev/null
+++ b/banned-die.h
@@ -0,0 +1,14 @@
+#ifndef BANNED_DIE_H
+#define BANNED_DIE_H
+
+#include "banned.h"
+
+/*
+ * This header lists functions that must not be used by low-level APIs
+ * because they can cause Git to terminate.
+ */
+
+#undef die
+#define die banned(die)
+
+#endif /* BANNED_DIE_H */
diff --git a/trace2.c b/trace2.c
index c23c0a227b..1d0ed2db2b 100644
--- a/trace2.c
+++ b/trace2.c
@@ -17,6 +17,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
+#include "banned-die.h"
 
 static int trace2_enabled;
 static int trace2_redact = 1;
diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
index bbcfeda60a..06912a3ceb 100644
--- a/trace2/tr2_cfg.c
+++ b/trace2/tr2_cfg.c
@@ -7,6 +7,7 @@
 #include "trace2/tr2_cfg.h"
 #include "trace2/tr2_sysenv.h"
 #include "wildmatch.h"
+#include "banned-die.h"
 
 static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;
 static int tr2_cfg_loaded;
diff --git a/trace2/tr2_cmd_name.c b/trace2/tr2_cmd_name.c
index b7b5a869b7..88f24e8781 100644
--- a/trace2/tr2_cmd_name.c
+++ b/trace2/tr2_cmd_name.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "trace2/tr2_cmd_name.h"
+#include "banned-die.h"
 
 #define TR2_ENVVAR_PARENT_NAME "GIT_TRACE2_PARENT_NAME"
 
diff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c
index ee17bfa86b..3067df4d18 100644
--- a/trace2/tr2_ctr.c
+++ b/trace2/tr2_ctr.c
@@ -2,6 +2,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_ctr.h"
+#include "banned-die.h"
 
 /*
  * A global counter block to aggregate values from the partial sums
diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
index 5be892cd5c..686a3e42fc 100644
--- a/trace2/tr2_dst.c
+++ b/trace2/tr2_dst.c
@@ -5,6 +5,7 @@
 #include "trace2/tr2_dst.h"
 #include "trace2/tr2_sid.h"
 #include "trace2/tr2_sysenv.h"
+#include "banned-die.h"
 
 /*
  * How many attempts we will make at creating an automatically-named trace file.
diff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c
index 1c1d27b0ee..358f61b301 100644
--- a/trace2/tr2_sid.c
+++ b/trace2/tr2_sid.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "trace2/tr2_tbuf.h"
 #include "trace2/tr2_sid.h"
+#include "banned-die.h"
 
 #define TR2_ENVVAR_PARENT_SID "GIT_TRACE2_PARENT_SID"
 
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index 4abc218514..deb3fabff4 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -4,6 +4,7 @@
 #include "config.h"
 #include "dir.h"
 #include "tr2_sysenv.h"
+#include "banned-die.h"
 
 /*
  * Each entry represents a trace2 setting.
diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c
index c3b3822ed7..86725426f6 100644
--- a/trace2/tr2_tbuf.c
+++ b/trace2/tr2_tbuf.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "tr2_tbuf.h"
+#include "banned-die.h"
 
 void tr2_tbuf_local_time(struct tr2_tbuf *tb)
 {
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 5a0381791f..a055e19bac 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -13,6 +13,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
+#include "banned-die.h"
 
 static struct tr2_dst tr2dst_event = {
 	.sysenv_var = TR2_SYSENV_EVENT,
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index 924736ab36..97d4c5d202 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -11,6 +11,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
+#include "banned-die.h"
 
 static struct tr2_dst tr2dst_normal = {
 	.sysenv_var = TR2_SYSENV_NORMAL,
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index 4eb9289f95..1f49d9f922 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -14,6 +14,7 @@
 #include "trace2/tr2_tgt.h"
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
+#include "banned-die.h"
 
 static struct tr2_dst tr2dst_perf = {
 	.sysenv_var = TR2_SYSENV_PERF,
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 7b023c1bfc..ae2d39d2f5 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -3,6 +3,7 @@
 #include "thread-utils.h"
 #include "trace.h"
 #include "trace2/tr2_tls.h"
+#include "banned-die.h"
 
 /*
  * Initialize size of the thread stack for nested regions.
diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c
index 038181ad9b..a329c466b9 100644
--- a/trace2/tr2_tmr.c
+++ b/trace2/tr2_tmr.c
@@ -3,6 +3,7 @@
 #include "trace2/tr2_tls.h"
 #include "trace2/tr2_tmr.h"
 #include "trace.h"
+#include "banned-die.h"
 
 #define MY_MAX(a, b) ((a) > (b) ? (a) : (b))
 #define MY_MIN(a, b) ((a) < (b) ? (a) : (b))
-- 
gitgitgadget

[PATCH v2 2/7] trace2: tolerate failed timestamp formatting

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

Some users reported issues of repeated messages:

  fatal: recursion detected in die handler

This wasn't happening every time, but we eventually captured a
GIT_TRACE2_PERF log file with this issue and revealed an interesting
internal detail, failing with this message:

  unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ

This specific format string tracks to tr2_tbuf_utc_datetime_extended()
in trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in
ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but
was later split in bad229aef23 (trace2: clarify UTC datetime formatting,
2019-04-15).

This use of xsnprintf() is writing a very specific datetime format into a
32-character buffer. The format requires that the input data will not
overflow the format digits or the buffer will not hold the result. Since
we are using xsnprintf() here, those failures turn into die() events.

This method and its siblings, tr2_tbuf_local_time() and
tr2_tbuf_utc_datetime(), are used in the tracing library. The extended
form is used only for the 'event' format, which these users were using
via a config setting for use in client-side telemetry. The non-extended
form is used to help generate the 'SID' that defines the process in the
traces.

Not only are these inappropriate times for a failure, but the extended
method is called specifially during the 'atexit' event, which was
triggering this problem in a loop as the 'atexit' event would be
retriggered by the die().

Based on other symptoms impacting users on the version reporting these
failures, it is most likely that this is actually a failure to allocate
memory, which is a specific symptom in Git for Windows. That fork uses a
different library for its implementation of vsprintf() which allocates
an array when seven or more positional arguments exist in the formatting
string, such as this one.

Ultimately, the trace2 machinery is so low-level that it should not rely on
any helper functions that perform error handling with die(), as that can
trigger issues that would then be traced, causing this kind of recursive
loop.

These changes help remove any use of die() within this file:

1. Both 'tv' and 'tm' structs are initialized with zero values, allowing
   an erroring gettimeofday() or gmtime_r() method to leave them
   zero-valued. A zero-valued date is better than a die() here.

2. Replace the use of xsnprintf() with snprintf() to avoid the
   possibility of calling die() here. Instead, check the response to see
   if there was a failure. On failure, put a blank value into the buffer
   instead of possibly allowing a value that would not format correctly
   for a trace2 consumer. This value should be seen as obviously wrong
   and therefore signals a problem.

As the core issue in this code seems to require a system method
returning an error, no test accompanies this change.

This change removes all uses of xsnprintf() from the trace2/ directory.
There are two uses of xstrdup() that could be considered for removal,
but they only die() on out-of-memory errors instead of formatting
issues. I chose to leave those in place for now.

Helped-by: Taylor Blau <ttaylorr@openai.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h      |  3 +++
 trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index 5eff361e55..0e0a794e5d 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -11,4 +11,7 @@
 #undef die
 #define die banned(die)
 
+#undef xsnprintf
+#define xsnprintf(...) BANNED(xsnprintf)
+
 #endif /* BANNED_DIE_H */
diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c
index 86725426f6..fff345cb99 100644
--- a/trace2/tr2_tbuf.c
+++ b/trace2/tr2_tbuf.c
@@ -4,45 +4,64 @@
 
 void tr2_tbuf_local_time(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	localtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld", tm.tm_hour,
-		  tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld",
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "00:00:00.000000";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }
 
 void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	gmtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf),
-		  "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ", tm.tm_year + 1900,
-		  tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,
-		  (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf),
+		       "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ",
+		       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "1900-00-00T00:00:00.000000Z";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }
 
 void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)
 {
-	struct timeval tv;
-	struct tm tm;
+	struct timeval tv = { 0 };
+	struct tm tm = { 0 };
 	time_t secs;
+	int len;
 
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	gmtime_r(&secs, &tm);
 
-	xsnprintf(tb->buf, sizeof(tb->buf), "%4d%02d%02dT%02d%02d%02d.%06ldZ",
-		  tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
-		  tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+	len = snprintf(tb->buf, sizeof(tb->buf),
+		       "%4d%02d%02dT%02d%02d%02d.%06ldZ",
+		       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		       tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+	if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+		const char *blank = "19000000T000000.000000Z";
+		strlcpy(tb->buf, blank, sizeof(tb->buf));
+	}
 }
-- 
gitgitgadget

[PATCH v2 3/7] trace2: remove use of xstrdup()

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

In the previous change, we removed a use of xsprintf() that caused a
recursive die() loop when failing to allocate memory. The trace2 library is
too low-level to be calling die(), especially because of these recursive
loops that can occur during the die handler.

For full defense in depth, we remove the xstrdup() calls from
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.

Second, in tr2_sysenv_get(), the method will return NULL when strdup()
returns NULL. This return is indistinguishable from the environment variable
having no value. That means that all callers know how to handle a NULL
response, but no behavior change will occur between the case of no
environment being set and detecting an environment variable exists but we
fail to duplicate it. This seems an appropriate trade-off, as an allocation
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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h        | 3 +++
 trace2/tr2_sysenv.c | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index 0e0a794e5d..2e16c4899c 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -14,4 +14,7 @@
 #undef xsnprintf
 #define xsnprintf(...) BANNED(xsnprintf)
 
+#undef xstrdup
+#define xstrdup(str) BANNED(xstrdup)
+
 #endif /* BANNED_DIE_H */
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index deb3fabff4..4ee273a4ae 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,
 			if (!value)
 				return config_error_nonbool(key);
 			free(tr2_sysenv_settings[k].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;
 		}
 	}
@@ -110,7 +112,7 @@ const char *tr2_sysenv_get(enum tr2_sysenv_variable var)
 		const char *v = getenv(tr2_sysenv_settings[var].env_var_name);
 		if (v && *v) {
 			free(tr2_sysenv_settings[var].value);
-			tr2_sysenv_settings[var].value = xstrdup(v);
+			tr2_sysenv_settings[var].value = strdup(v);
 		}
 		tr2_sysenv_settings[var].getenv_called = 1;
 	}
-- 
gitgitgadget

[PATCH v2 4/7] trace2: remove use of ALLOC_ARRAY()

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

The banned-die.h header is used to prevent use of helper methods that
call die(). Remove use of the ALLOC_ARRAY() helper, which calls die() on
allocation failures. Replace the use in trace2.c with a more direct
allocation and soft failure when allocation fails. This prevents die()
recursion loops when memory allocation fails and trace2 logs are
enabled.

The tricky part about this change is how to handle the results from
redact_arg(), which is a 'const char *' result because it might be a
pointer directly to the externally-controlled argument. When it is
different from the argument, then it is indeed a newly-allocated string
that we need to free before returning. This requires using a (char *)
cast to allow a change.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h |  3 +++
 trace2.c     | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index 2e16c4899c..cb2eed75cd 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -17,4 +17,7 @@
 #undef xstrdup
 #define xstrdup(str) BANNED(xstrdup)
 
+#undef ALLOC_ARRAY
+#define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)
+
 #endif /* BANNED_DIE_H */
diff --git a/trace2.c b/trace2.c
index 1d0ed2db2b..7044276435 100644
--- a/trace2.c
+++ b/trace2.c
@@ -304,7 +304,11 @@ static const char **redact_argv(const char **argv)
 	for (j = 0; argv[j]; j++)
 		; /* keep counting */
 
-	ALLOC_ARRAY(ret, j + 1);
+	ret = calloc(j + 1, sizeof(*ret));
+	if (!ret) {
+		free((char *)redacted);
+		return NULL;
+	}
 	ret[j] = NULL;
 
 	for (j = 0; j < i; j++)
@@ -345,6 +349,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
 	us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
 
 	redacted = redact_argv(argv);
+	if (!redacted)
+		return;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_start_fl)
@@ -513,6 +519,7 @@ void trace2_child_start_fl(const char *file, int line,
 	uint64_t us_now;
 	uint64_t us_elapsed_absolute;
 	const char **orig_argv = cmd->args.v;
+	const char **redacted;
 
 	if (!trace2_enabled)
 		return;
@@ -530,7 +537,10 @@ void trace2_child_start_fl(const char *file, int line,
 	 * temporarily replace the original argv (inside the `strvec`)
 	 * with a possibly redacted version.
 	 */
-	cmd->args.v = redact_argv(orig_argv);
+	redacted = redact_argv(orig_argv);
+	if (!redacted)
+		return;
+	cmd->args.v = redacted;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_child_start_fl)
@@ -622,6 +632,8 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
 	exec_id = tr2tls_locked_increment(&tr2_next_exec_id);
 
 	redacted = redact_argv(argv);
+	if (!redacted)
+		return exec_id;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_exec_fl)
-- 
gitgitgadget

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

[Derrick Stolee via GitGitGadget]

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.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h |  3 +++
 trace2.c     | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index cb2eed75cd..14aecfdc7a 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/trace2.c b/trace2.c
index 7044276435..c37f783fa0 100644
--- a/trace2.c
+++ b/trace2.c
@@ -260,7 +260,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) &&
@@ -275,7 +278,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))
+		return NULL;
+
+	redacted_len = prefix_len + suffix_len + redact_len;
+
+	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);
+	return redacted;
 }
 
 /*
@@ -300,6 +321,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 */
@@ -316,7 +339,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

[PATCH v2 6/7] trace2: remove use of ALLOC_GROW()

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

The ALLOC_GROW() helper can call die() on a failed memory allocation.
We need to remove this from the trace2 API code to prevent a recursive
die() handler.

This helper is used to track the nested region stack. Use a new
skipped_regions member to track how many times a region was entered
without being added to the stack, and decrease that amount as we leave
each region. This allows us to avoid a failure and instead stop
deepening the stack, giving as much nesting behavior as possible without
failing the entire process.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h     |  3 +++
 trace2/tr2_tls.c | 34 +++++++++++++++++++++++++++++++++-
 trace2/tr2_tls.h |  1 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/banned-die.h b/banned-die.h
index 14aecfdc7a..423e7b607d 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -23,4 +23,7 @@
 #undef ALLOC_ARRAY
 #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)
 
+#undef ALLOC_GROW
+#define ALLOC_GROW(x, nr, alloc) BANNED(ALLOC_GROW)
+
 #endif /* BANNED_DIE_H */
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index ae2d39d2f5..8596292a94 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -108,8 +108,33 @@ void tr2tls_unset_self(void)
 void tr2tls_push_self(uint64_t us_now)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
+	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)
+		return;
+
+	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;
 
-	ALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc);
 	ctx->array_us_start[ctx->nr_open_regions++] = us_now;
 }
 
@@ -117,6 +142,11 @@ void tr2tls_pop_self(void)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 
+	if (ctx->nr_skipped_regions) {
+		ctx->nr_skipped_regions--;
+		return;
+	}
+
 	if (!ctx->nr_open_regions)
 		BUG("no open regions in thread '%s'", ctx->thread_name);
 
@@ -137,6 +167,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)
 	uint64_t us_start;
 
 	ctx = tr2tls_get_self();
+	if (ctx->nr_skipped_regions)
+		return 0;
 	if (!ctx->nr_open_regions)
 		return 0;
 
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index 3bdbf4d275..c365017923 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -20,6 +20,7 @@ struct tr2tls_thread_ctx {
 	uint64_t *array_us_start;
 	size_t alloc;
 	size_t nr_open_regions; /* plays role of "nr" in ALLOC_GROW */
+	size_t nr_skipped_regions;
 	int thread_id;
 	struct tr2_timer_block timer_block;
 	struct tr2_counter_block counter_block;
-- 
gitgitgadget

[PATCH v2 7/7] trace2: remove use of xcalloc()

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <stolee@gmail.com>

Remove use of xcalloc() from the trace2 API due to its possible use of
die(), which could lead to recursive die() handlers. This is used in the
trace2 API to track an array of thread contexts when logging multi-
threaded operations.

Instead of killing the process on a failure, we attempt to proceed as
much as possible. We replace the dynamic thread context with a
statically-allocated context that uses the "unknown" thread name to
identify that we are in an error case.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 banned-die.h     |  3 ++
 trace2/tr2_ctr.c | 10 ++++++-
 trace2/tr2_tls.c | 73 ++++++++++++++++++++++++++++++++++++------------
 trace2/tr2_tls.h |  6 ++++
 trace2/tr2_tmr.c | 14 ++++++++--
 5 files changed, 85 insertions(+), 21 deletions(-)

diff --git a/banned-die.h b/banned-die.h
index 423e7b607d..3dc521f6b0 100644
--- a/banned-die.h
+++ b/banned-die.h
@@ -17,6 +17,9 @@
 #undef xstrdup
 #define xstrdup(str) BANNED(xstrdup)
 
+#undef xcalloc
+#define xcalloc(nmemb, size) BANNED(xcalloc)
+
 #undef xstrfmt
 #define xstrfmt(...) BANNED(xstrfmt)
 
diff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c
index 3067df4d18..5283946e08 100644
--- a/trace2/tr2_ctr.c
+++ b/trace2/tr2_ctr.c
@@ -54,7 +54,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER
 void tr2_counter_increment(enum trace2_counter_id cid, uint64_t value)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	struct tr2_counter *c = &ctx->counter_block.counter[cid];
+	struct tr2_counter *c;
+
+	if (tr2tls_is_fallback(ctx))
+		return;
+	c = &ctx->counter_block.counter[cid];
 
 	c->value += value;
 
@@ -68,6 +72,8 @@ void tr2_update_final_counters(void)
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 	enum trace2_counter_id cid;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
 	if (!ctx->used_any_counter)
 		return;
 
@@ -89,6 +95,8 @@ void tr2_emit_per_thread_counters(tr2_tgt_evt_counter_t *fn_apply)
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 	enum trace2_counter_id cid;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
 	if (!ctx->used_any_per_thread_counter)
 		return;
 
diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 8596292a94..2c6aaed504 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -13,6 +13,9 @@
 #define TR2_REGION_NESTING_INITIAL_SIZE (100)
 
 static struct tr2tls_thread_ctx *tr2tls_thread_main;
+static struct tr2tls_thread_ctx tr2tls_thread_fallback = {
+	.thread_name = "unknown",
+};
 static uint64_t tr2tls_us_start_process;
 
 static pthread_mutex_t tr2tls_mutex;
@@ -37,16 +40,23 @@ void tr2tls_start_process_clock(void)
 struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
 					     uint64_t us_thread_start)
 {
-	struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
+	struct tr2tls_thread_ctx *ctx = calloc(1, sizeof(*ctx));
 	struct strbuf buf = STRBUF_INIT;
 
+	if (!ctx)
+		goto fallback;
+
 	/*
 	 * Implicitly "tr2tls_push_self()" to capture the thread's start
 	 * time in array_us_start[0].  For the main thread this gives us the
 	 * application run time.
 	 */
 	ctx->alloc = TR2_REGION_NESTING_INITIAL_SIZE;
-	ctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t));
+	ctx->array_us_start = calloc(ctx->alloc, sizeof(uint64_t));
+	if (!ctx->array_us_start) {
+		free(ctx);
+		goto fallback;
+	}
 	ctx->array_us_start[ctx->nr_open_regions++] = us_thread_start;
 
 	ctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);
@@ -62,6 +72,10 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
 	pthread_setspecific(tr2tls_key, ctx);
 
 	return ctx;
+
+fallback:
+	pthread_setspecific(tr2tls_key, &tr2tls_thread_fallback);
+	return &tr2tls_thread_fallback;
 }
 
 struct tr2tls_thread_ctx *tr2tls_get_self(void)
@@ -84,6 +98,11 @@ struct tr2tls_thread_ctx *tr2tls_get_self(void)
 	return ctx;
 }
 
+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx)
+{
+	return ctx == &tr2tls_thread_fallback;
+}
+
 int tr2tls_is_main_thread(void)
 {
 	if (!HAVE_THREADS)
@@ -100,6 +119,9 @@ void tr2tls_unset_self(void)
 
 	pthread_setspecific(tr2tls_key, NULL);
 
+	if (tr2tls_is_fallback(ctx))
+		return;
+
 	free((char *)ctx->thread_name);
 	free(ctx->array_us_start);
 	free(ctx);
@@ -111,30 +133,33 @@ 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) {
 		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;
 }
 
@@ -142,6 +167,9 @@ void tr2tls_pop_self(void)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 
+	if (tr2tls_is_fallback(ctx))
+		return;
+
 	if (ctx->nr_skipped_regions) {
 		ctx->nr_skipped_regions--;
 		return;
@@ -157,6 +185,9 @@ void tr2tls_pop_unwind_self(void)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 
+	if (tr2tls_is_fallback(ctx))
+		return;
+
 	while (ctx->nr_open_regions > 1)
 		tr2tls_pop_self();
 }
@@ -167,6 +198,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)
 	uint64_t us_start;
 
 	ctx = tr2tls_get_self();
+	if (tr2tls_is_fallback(ctx))
+		return 0;
 	if (ctx->nr_skipped_regions)
 		return 0;
 	if (!ctx->nr_open_regions)
@@ -188,6 +221,10 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)
 static void tr2tls_key_destructor(void *payload)
 {
 	struct tr2tls_thread_ctx *ctx = payload;
+
+	if (tr2tls_is_fallback(ctx))
+		return;
+
 	free((char *)ctx->thread_name);
 	free(ctx->array_us_start);
 	free(ctx);
diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h
index c365017923..4a0969c014 100644
--- a/trace2/tr2_tls.h
+++ b/trace2/tr2_tls.h
@@ -54,6 +54,12 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,
  */
 struct tr2tls_thread_ctx *tr2tls_get_self(void);
 
+/*
+ * Return true if the context is the non-allocating fallback used after an
+ * allocation failure. Callers must not modify a fallback context.
+ */
+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx);
+
 /*
  * return true if the current thread is the main thread.
  */
diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c
index a329c466b9..b3d26e2b31 100644
--- a/trace2/tr2_tmr.c
+++ b/trace2/tr2_tmr.c
@@ -38,8 +38,11 @@ static struct tr2_timer_metadata tr2_timer_metadata[TRACE2_NUMBER_OF_TIMERS] = {
 void tr2_start_timer(enum trace2_timer_id tid)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	struct tr2_timer *t = &ctx->timer_block.timer[tid];
+	struct tr2_timer *t;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
+	t = &ctx->timer_block.timer[tid];
 	t->recursion_count++;
 	if (t->recursion_count > 1)
 		return; /* ignore recursive starts */
@@ -50,10 +53,13 @@ void tr2_start_timer(enum trace2_timer_id tid)
 void tr2_stop_timer(enum trace2_timer_id tid)
 {
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	struct tr2_timer *t = &ctx->timer_block.timer[tid];
+	struct tr2_timer *t;
 	uint64_t ns_now;
 	uint64_t ns_interval;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
+	t = &ctx->timer_block.timer[tid];
 	assert(t->recursion_count > 0);
 
 	t->recursion_count--;
@@ -91,6 +97,8 @@ void tr2_update_final_timers(void)
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 	enum trace2_timer_id tid;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
 	if (!ctx->used_any_timer)
 		return;
 
@@ -137,6 +145,8 @@ void tr2_emit_per_thread_timers(tr2_tgt_evt_timer_t *fn_apply)
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
 	enum trace2_timer_id tid;
 
+	if (tr2tls_is_fallback(ctx))
+		return;
 	if (!ctx->used_any_per_thread_timer)
 		return;
 
-- 
gitgitgadget

Re: [PATCH v2 1/7] banned-die: create header for banning of functions

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <stolee@gmail.com>
>
> We have universally-banned functions listed in banned.h since
> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of
> the code should be more strict than others.
>
> One such example is the trace2 API which runs during atexit() and can
> prove to cause die()-handler recursion problems if it calls die().
>
> Create a new banned-die.h header file that will ban some Git methods
> that call die(). Include that in all trace2 API implementation files.
> This currently only bans die() itself, and that was already not used.
>
> It would be reasonable to name this file trace2/tr2_banned.h to be
> specific to the trace2 API, but it seems like such a restriction would
> be valuable to put in some other areas of the code, so adding it at the
> root of the tree seems like a good long-term approach.

In other words, the functions banned by including this file are not
listed because they are banned from being used in trace2 API, but
because they may lead to die().  There may be some other traits that
we might want to avoid in certain subset of our code, and we may
have similar banned-frotz.h header to prevent direct or indirect use
of frotz.  Which makes sense to me.

Would the same approach work for the_hash_algo and the_repository, I
wonder?

Re: [PATCH v2 1/7] banned-die: create header for banning of functions

[Elijah Newren]

On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
[...]
> +#undef die
> +#define die banned(die)

Shouldn't that be BANNED(die) to match all the other cases in the code
(and avoid an obtuse "implicit declaration of function 'banned'"
instead of the nicer "sorry_die_is_a_banned_function" message)?

> +
> +#endif /* BANNED_DIE_H */
> diff --git a/trace2.c b/trace2.c
> index c23c0a227b..1d0ed2db2b 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -17,6 +17,7 @@
>  #include "trace2/tr2_tgt.h"
>  #include "trace2/tr2_tls.h"
>  #include "trace2/tr2_tmr.h"
> +#include "banned-die.h"
>

Is there a risk that future folks add new includes at the end of the
list, then functions in them get added to banned-die.h, but are
silently ignored because banned-die.h wasn't the last include?

Re: [PATCH v2 3/7] trace2: remove use of xstrdup()

[Elijah Newren]

On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
[...]
> For full defense in depth, we remove the xstrdup() calls from
> 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.
>
[...]
> --- a/trace2/tr2_sysenv.c
> +++ b/trace2/tr2_sysenv.c
> @@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,
>                         if (!value)
>                                 return config_error_nonbool(key);
>                         free(tr2_sysenv_settings[k].value);
> -                       tr2_sysenv_settings[k].value = xstrdup(value);
> +                       tr2_sysenv_settings[k].value = strdup(value);
> +                       if (!tr2_sysenv_settings[k].value)
> +                               return -1;

I'm not sure if this matters, but I think the call sequence from
config.c to this function is:

  read_very_early_config ->
    config_with_options ->
      git_config_from_file_with_options ->
        do_config_from_file ->
          do_config_from ->
            git_parse_source ->
              get_value ->
                git_config_include ->
                  tr2_sysenv_cb

and the -1 unwinds back to git_parse_source, which breaks, formats an
error message, and calls die:

   error_msg = xstrfmt(_("bad config line %d in file %s")...)
   die("%s", error_msg)

Am I reading this right?  If so, the -1 actually triggers a die as
well -- unless the allocation in xstrfmt manages to kill it first.
This isn't a regression (the old xstrdup() also died) and the die
isn't inside the trace functions, but the commit message might read as
promising more than it delivers.

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

[Elijah Newren]

On Tue, Aug 25, 2026 at 11:59 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
[...]
>+       const char *redact = ":<REDACTED>";
>+       char *redacted;
[...]
> +       memcpy(redacted, arg, prefix_len);
> +       memcpy(redacted + prefix_len, redact, redact_len - 1);

Only copy redact_len - 1 bytes?  So only ":<REDACTED" without the
trailing ">" ?  Why?


> +       memcpy(redacted + prefix_len + redact_len - 1, p + at,
> +              suffix_len + 1);
> +       return redacted;
>  }
>

Re: [PATCH v2 6/7] trace2: remove use of ALLOC_GROW()

[Elijah Newren]

On Tue, Aug 25, 2026 at 11:57 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <stolee@gmail.com>
>
> The ALLOC_GROW() helper can call die() on a failed memory allocation.
> We need to remove this from the trace2 API code to prevent a recursive
> die() handler.
>
> This helper is used to track the nested region stack. Use a new
> skipped_regions member to track how many times a region was entered
> without being added to the stack, and decrease that amount as we leave
> each region. This allows us to avoid a failure and instead stop
> deepening the stack, giving as much nesting behavior as possible without
> failing the entire process.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>

Checking out this commit and running

   GIT_TRACE2_PERF=1 ./bin-wrappers/git status

dies with

   no open regions in thread 'main'

Seems to be fixed by 7/7, though.  Maybe a bad splitting?

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Tue, Aug 25, 2026 at 11:59 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
> [...]
>>+       const char *redact = ":<REDACTED>";
>>+       char *redacted;
> [...]
>> +       memcpy(redacted, arg, prefix_len);
>> +       memcpy(redacted + prefix_len, redact, redact_len - 1);
>
> Only copy redact_len - 1 bytes?  So only ":<REDACTED" without the
> trailing ">" ?  Why?

Yeah, if it were (redact_len + 1) it would have worked better, perhaps?

>
>
>> +       memcpy(redacted + prefix_len + redact_len - 1, p + at,
>> +              suffix_len + 1);
>> +       return redacted;
>>  }
>>

Re: [PATCH v2 1/7] banned-die: create header for banning of functions

[Jeff King]

On Tue, Aug 25, 2026 at 06:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:

> We have universally-banned functions listed in banned.h since
> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of
> the code should be more strict than others.
> 
> One such example is the trace2 API which runs during atexit() and can
> prove to cause die()-handler recursion problems if it calls die().
> 
> Create a new banned-die.h header file that will ban some Git methods
> that call die(). Include that in all trace2 API implementation files.
> This currently only bans die() itself, and that was already not used.

There's a subtle but big difference between the universal code bans in
banned.h and this banned-die.h. In the former case we are deciding
strcpy() is unfit for our code base and outlawing it everywhere. The
potential problem is in the source code, so catching it while compiling
the source code is OK.

But we are not doing that with die(). It is a perfectly OK function in
general, but we do not want to ever trigger its runtime effects from
certain code paths. Banning it from being called from those code paths
can catch _some_ instances, but not any transitive calls. If we call
foo(), it may call die() itself, and we would not want to ban foo() from
doing so. And recursively for functions called by foo() and so on.

So you end up playing whack-a-mole with functions that might call die()
and adding them to this ban list.

I think that's _probably_ the best we can do in practice. I think the
framing above suggests that we could approach the problem more directly
with a runtime flag: when we enter those code paths, set a flag to avoid
the unwanted behavior, and have the low-level code respect that. But
die() is a special case here, because we'd want to suppress its
no-return behavior. And its callers are not prepared for die() to
suddenly start returning because of some global flag.

So I think the whack-a-mole is the best we can do. But I would not want
to see this strategy extended to other areas. In most cases some kind of
runtime support is probably a better solution.

-Peff

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

[Jeff King]

On Tue, Aug 25, 2026 at 06:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:

> 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()

OK. This feels like the tip of the iceberg, though. All of strbuf would
have to be off-limits, too (both because it calls malloc directly, but
also because it will bail if snprintf() returns -1). I won't be
surprised if there are other indirect calls hiding in various places
(e.g., all of json-writer.c).

I think if you really want to avoid allocations in trace2 it would
probably need to be a ground-up no-dependency rewrite.

-Peff