trace2: stop allowing die()

[derrickstolee]

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

cc: gitster@pobox.com
cc: Taylor Blau ttaylorr@openai.com
cc: Elijah Newren newren@gmail.com

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.2178.git.1784131932489.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2178/derrickstolee/trace2-dont-die-v1

To fetch this version to local tag pr-2178/derrickstolee/trace2-dont-die-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2178/derrickstolee/trace2-dont-die-v1

[gitgitgadget]

This branch is now known as ds/trace2-tolerate-failed-timestamp.

[gitgitgadget]

This patch series was integrated into seen via git@a124029.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The trace2 telemetry library has been updated to tolerate failures
from system calls like 'gettimeofday()' and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Needs review.
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

Taylor Blau wrote on the Git mailing list (how to reply to this email):

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

[gitgitgadget]

User Taylor Blau <ttaylorr@openai.com> has been added to the cc: list.

[gitgitgadget]

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

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

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like 'gettimeofday()' and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <alpXW5U6sndZtgqV@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

[gitgitgadget]

Taylor Blau wrote on the Git mailing list (how to reply to this email):

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

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like 'gettimeofday()' and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <xmqqzezlhgyo.fsf@gitster.g>
cf. <al4yrXXoZiHLwSvE@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <xmqqzezlhgyo.fsf@gitster.g>
cf. <al4yrXXoZiHLwSvE@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <xmqqzezlhgyo.fsf@gitster.g>
cf. <al4yrXXoZiHLwSvE@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <alpXW5U6sndZtgqV@com-79390>
cf. <c8d443a5-3cfb-4752-8716-cf0d8fadd9d3@gmail.com>
cf. <xmqqzezlhgyo.fsf@gitster.g>
cf. <al4yrXXoZiHLwSvE@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Waiting for response.
cf. <xmqqzezlhgyo.fsf@gitster.g>
cf. <al4yrXXoZiHLwSvE@com-79390>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

[gitgitgadget]

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

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

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <alpXW5U6sndZtgqV@com-79390>
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
cf. <xmqqpl03cfua.fsf@gitster.g>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <alpXW5U6sndZtgqV@com-79390>
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There are issues in commit cd9365c:
trace2: add mechanism to ban functions

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There are issues in commit 1c38e98:
trace2: remove use of xgethostname()

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There are issues in commit 4e20f16:
trace2: remove use of ALLOC_ARRAY()

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There are issues in commit 8dfa464:
trace2: remove use of xstrfmt()

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There are issues in commit e72ed12:
trace2: remove use of ALLOC_GROW()

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There are issues in commit 7782924:
trace2: remove use of xcalloc()

• Commit not signed off
• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ds/trace2-tolerate-failed-timestamp on the Git mailing list:

The 'trace2' telemetry library has been updated to tolerate failures
from system calls like gettimeofday() and datetime formatting
functions, replacing potential program crashes with blank placeholder
timestamps in the traces.

Expecting a reroll.
cf. <fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com>
source: <pull.2178.git.1784131932489.gitgitgadget@gmail.com>

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.2178.v2.git.1787684181.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2178/derrickstolee/trace2-dont-die-v2

To fetch this version to local tag pr-2178/derrickstolee/trace2-dont-die-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2178/derrickstolee/trace2-dont-die-v2

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"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?

[gitgitgadget]

Elijah Newren wrote on the Git mailing list (how to reply to this email):

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?

[gitgitgadget]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[gitgitgadget]

Elijah Newren wrote on the Git mailing list (how to reply to this email):

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.

[gitgitgadget]

Elijah Newren wrote on the Git mailing list (how to reply to this email):

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;
>  }
>

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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;
>>  }
>>

[gitgitgadget]

Elijah Newren wrote on the Git mailing list (how to reply to this email):

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?