{"thread":{"id":"66006","subject":"[PATCH] trace2: tolerate failed timestamp formatting","startedAt":"2026-07-15T16:12:17Z","lastAt":"2026-09-01T13:42:50Z","messageCount":42,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Derrick Stolee","Junio C Hamano","Elijah Newren","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548304","messageId":"pull.2178.git.1784131932489.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":null,"subject":"[PATCH] trace2: tolerate failed timestamp formatting","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-15T16:12:11Z","receivedAt":"2026-07-15T16:12:17Z","isPatch":true,"body":"From: Derrick Stolee \n\nSome users reported issues of repeated messages:\n\n fatal: recursion detected in die handler\n\nThis wasn't happening every time, but we eventually captured a\nGIT_TRACE2_PERF log file with this issue and revealed an interesting\ninternal detail, failing with this message:\n\n unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\n\nThis specific format string tracks to tr2_tbuf_utc_datetime_extended()\nin trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but\nwas later split in bad229aef23 (trace2: clarify UTC datetime formatting,\n2019-04-15).\n\nThis use of xsnprintf() is writing a very specific datetime format into a\n32-character buffer. The format requires that the input data will not\noverflow the format digits or the buffer will not hold the result. Since\nwe are using xsnprintf() here, those failures turn into die() events.\n\nThis method and its siblings, tr2_tbuf_local_time() and\ntr2_tbuf_utc_datetime(), are used in the tracing library. The extended\nform is used only for the 'event' format, which these users were using\nvia a config setting for use in client-side telemetry. The non-extended\nform is used to help generate the 'SID' that defines the process in the\ntraces.\n\nNot only are these inappropriate times for a failure, but the extended\nmethod is called specifially during the 'atexit' event, which was\ntriggering this problem in a loop as the 'atexit' event would be\nretriggered by the die().\n\nI could not determine the exact cause of why these errors started\noccuring in a bunch. My best guess is that these users are dogfooding an\nearly operating system version that is more likely to fail in the\ngettimeofday() function and thus leaves the structures uninitialized and\npotentially violating the expected values.\n\nHowever, for full defense-in-depth I made several modifications:\n\n1. Both 'tv' and 'tm' structs are initialized with zero values, allowing\n an erroring gettimeofday() or gmtime_r() method to leave them\n zero-valued. A zero-valued date is better than a die() here.\n\n2. Replace the use of xsnprintf() with snprintf() to avoid the\n possibility of calling die() here. Instead, check the response to see\n if there was a failure. On failure, put a blank value into the buffer\n instead of possibly allowing a value that would not format correctly\n for a trace2 consumer. This value should be seen as obviously wrong\n and therefore signals a problem.\n\nAs the core issue in this code seems to require a system method\nreturning an error, no test accompanies this change.\n\nThis change removes all uses of xsnprintf() from the trace2/ directory.\nThere are two uses of xstrdup() that could be considered for removal,\nbut they only die() on out-of-memory errors instead of formatting\nissues. I chose to leave those in place for now.\n\nSigned-off-by: Derrick Stolee \n---\n trace2: tolerate failed timestamp formatting\n \n As mentioned, this is based on real trace logs of failed commands users\n are seeing.\n \n I wish I had a better way to test this or to be 100% sure that the\n system call was failing. But users were seeing failures and these seemed\n like appropriate changes.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2178\n\n trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------\n 1 file changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c\nindex c3b3822ed7..ef57376f3c 100644\n--- a/trace2/tr2_tbuf.c\n+++ b/trace2/tr2_tbuf.c\n@@ -3,45 +3,64 @@\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tlocaltime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\", tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\",\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"00:00:00.000000\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf),\n-\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\", tm.tm_year + 1900,\n-\t\t tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,\n-\t\t (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"1900-00-00T00:00:00.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n-\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"19000000T000000.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \ngitgitgadget\n"},{"id":"548545","messageId":"alpXW5U6sndZtgqV@com-79390","threadId":"66006","inReplyTo":"pull.2178.git.1784131932489.gitgitgadget@gmail.com","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-17T16:24:59Z","receivedAt":"2026-07-17T16:25:03Z","isPatch":true,"body":"On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:\n> This change removes all uses of xsnprintf() from the trace2/ directory.\n> There are two uses of xstrdup() that could be considered for removal,\n> but they only die() on out-of-memory errors instead of formatting\n> issues. I chose to leave those in place for now.\n\nI may be missing some Git for Windows context, but I dug into this a\nlittle and I'm not sure 'gettimeofday()' is the culprit...\n\nIn my understanding Git for Windows's 'gettext.h' appears[1] to redirect\nthe 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this\ncase, we have seven '%' placeholders. Gettext can store only six plus\nits end marker inline, so parsing the seventh causes an allocation\nbefore any timestamp values are read.\n\nA failure there would produce the observed -1, after which 'xsnprintf()'\ndies and trace2 can recurse.\n\nI think that also explains why calling 'snprintf()' directly helps.\ntr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If\nI'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and\nalways returns zero [2], making the zero-initialization unrelated.\n\nWould it make more sense to fix the xsnprintf()/libintl boundary and\ntreat Trace2 reentrancy separately? I still can't explain why the\nallocation failed, so there may be another GfW-specific piece I’m\nmissing.\n\nI think something like the following (untested) would prevent the\nredirection to `libintl_vsnprintf()`:\n\n--- 8< ---\ndiff --git a/wrapper.c b/wrapper.c\nindex 16f5a63fbb..2976d4e110 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -7,7 +7,14 @@\n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n #include \"parse.h\"\n+\n+/*\n+ * xsnprintf() only formats non-translated strings. On MinGW, avoid\n+ * redirecting its vsnprintf() call to libintl's allocating replacement.\n+ */\n+#define _INTL_NO_DEFINE_MACRO_VSNPRINTF\n #include \"gettext.h\"\n+#undef _INTL_NO_DEFINE_MACRO_VSNPRINTF\n #include \"strbuf.h\"\n #include \"trace2.h\"\n--- >8 ---\n\nThanks,\nTaylor\n\n[1]: https://github.com/git-for-windows/git-sdk-64/blob/1351ad2fc39a1f74c56b2cc2b38107ec8df8eb40/mingw64/include/libintl.h#L731-L754\n[2]: https://github.com/microsoft/git/blob/vfs-2.55.0/compat/mingw.c#L1609-L1618\n"},{"id":"548586","messageId":"c8d443a5-3cfb-4752-8716-cf0d8fadd9d3@gmail.com","threadId":"66006","inReplyTo":"alpXW5U6sndZtgqV@com-79390","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-07-18T15:01:02Z","receivedAt":"2026-07-18T15:01:06Z","isPatch":true,"body":"On 7/17/2026 12:24 PM, Taylor Blau wrote:\n> On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> This change removes all uses of xsnprintf() from the trace2/ directory.\n>> There are two uses of xstrdup() that could be considered for removal,\n>> but they only die() on out-of-memory errors instead of formatting\n>> issues. I chose to leave those in place for now.\n> \n> I may be missing some Git for Windows context, but I dug into this a\n> little and I'm not sure 'gettimeofday()' is the culprit...\n> \n> In my understanding Git for Windows's 'gettext.h' appears[1] to redirect\n> the 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this\n> case, we have seven '%' placeholders. Gettext can store only six plus\n> its end marker inline, so parsing the seventh causes an allocation\n> before any timestamp values are read.\n> \n> A failure there would produce the observed -1, after which 'xsnprintf()'\n> dies and trace2 can recurse.\n\nWith this perspective, the issue is that gettext is doing dynamic\nallocation and getting a failure there, which explains the transient\nnature. This is an interesting idea, and a more likely \"application\nside\" error. I'm still curious why this is creeping up for the first\ntime in this burst, since nothing has changed in the application, to\nmy knowledge. \n> I think that also explains why calling 'snprintf()' directly helps.\n> tr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If\n> I'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and\n> always returns zero [2], making the zero-initialization unrelated.\n> \n> Would it make more sense to fix the xsnprintf()/libintl boundary and\n> treat Trace2 reentrancy separately? I still can't explain why the\n> allocation failed, so there may be another GfW-specific piece I’m\n> missing.\n\nI think that your suggested change has merits and should be pursued.\nI'll explore it a bit to confirm.\n\nThe other justification I'd like to make in my patch is that the\nxsnprintf() calls die() and the trace2 machinery should be die()-free\nwhenever possible. Solving both possible causes is likely the right\nlong-term approach.\n\nThanks,\n-Stolee\n\n\n"},{"id":"548682","messageId":"xmqqzezlhgyo.fsf@gitster.g","threadId":"66006","inReplyTo":"c8d443a5-3cfb-4752-8716-cf0d8fadd9d3@gmail.com","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-20T14:29:51Z","receivedAt":"2026-07-20T14:29:54Z","isPatch":true,"body":"Derrick Stolee writes:\n\n>> Would it make more sense to fix the xsnprintf()/libintl boundary and\n>> treat Trace2 reentrancy separately? I still can't explain why the\n>> allocation failed, so there may be another GfW-specific piece I’m\n>> missing.\n>\n> I think that your suggested change has merits and should be pursued.\n> I'll explore it a bit to confirm.\n\nThat band-aid may be a good idea, but I would prefer not to see the\nconditional in a common source file like 'wrapper.c'. Somewhere\nMinGW-specific would be more appropriate, would it not?\n\n> The other justification I'd like to make in my patch is that the\n> xsnprintf() calls die() and the trace2 machinery should be die()-free\n> whenever possible. Solving both possible causes is likely the right\n> long-term approach.\n\nThat is indeed worth considering.\n\nYou mention a few calls to xstrdup() that can potentially abort, and\nI agree that anything that triggers malloc() and notices that we are\nout of memory can probably do little better than to die. But are\nthere other operations that may cause us to exit, even though we are\nnot in an unrecoverable state (such as an out-of-memory condition)?\n\nThanks.\n"},{"id":"548683","messageId":"al4yrXXoZiHLwSvE@com-79390","threadId":"66006","inReplyTo":"xmqqzezlhgyo.fsf@gitster.g","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-07-20T14:37:33Z","receivedAt":"2026-07-20T14:37:38Z","isPatch":true,"body":"On Mon, Jul 20, 2026 at 07:29:51AM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>\n> >> Would it make more sense to fix the xsnprintf()/libintl boundary and\n> >> treat Trace2 reentrancy separately? I still can't explain why the\n> >> allocation failed, so there may be another GfW-specific piece I’m\n> >> missing.\n> >\n> > I think that your suggested change has merits and should be pursued.\n> > I'll explore it a bit to confirm.\n>\n> That band-aid may be a good idea, but I would prefer not to see the\n> conditional in a common source file like 'wrapper.c'. Somewhere\n> MinGW-specific would be more appropriate, would it not?\n\nYeah, to be clear, I do not think that putting the '#define' here in\n'wrapper.c' is appropriate, and included it in my original email only to\ndemonstrate the shape of the proposed solution.\n\nThanks,\nTaylor\n"},{"id":"549236","messageId":"xmqqh5lho4xc.fsf@gitster.g","threadId":"66006","inReplyTo":"xmqqzezlhgyo.fsf@gitster.g","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-29T21:35:11Z","receivedAt":"2026-07-29T21:35:13Z","isPatch":true,"body":"Junio C Hamano writes:\n\n> Derrick Stolee writes:\n>\n>>> Would it make more sense to fix the xsnprintf()/libintl boundary and\n>>> treat Trace2 reentrancy separately? I still can't explain why the\n>>> allocation failed, so there may be another GfW-specific piece I’m\n>>> missing.\n>>\n>> I think that your suggested change has merits and should be pursued.\n>> I'll explore it a bit to confirm.\n>\n> That band-aid may be a good idea, but I would prefer not to see the\n> conditional in a common source file like 'wrapper.c'. Somewhere\n> MinGW-specific would be more appropriate, would it not?\n\nDid anything come out of this discussion?\n\n>\n>> The other justification I'd like to make in my patch is that the\n>> xsnprintf() calls die() and the trace2 machinery should be die()-free\n>> whenever possible. Solving both possible causes is likely the right\n>> long-term approach.\n>\n> That is indeed worth considering.\n>\n> You mention a few calls to xstrdup() that can potentially abort, and\n> I agree that anything that triggers malloc() and notices that we are\n> out of memory can probably do little better than to die. But are\n> there other operations that may cause us to exit, even though we are\n> not in an unrecoverable state (such as an out-of-memory condition)?\n>\n> Thanks.\n"},{"id":"549352","messageId":"fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com","threadId":"66006","inReplyTo":"xmqqh5lho4xc.fsf@gitster.g","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-07-31T13:26:38Z","receivedAt":"2026-07-31T13:26:40Z","isPatch":true,"body":"On 7/29/2026 5:35 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Derrick Stolee writes:\n>>\n>>>> Would it make more sense to fix the xsnprintf()/libintl boundary and\n>>>> treat Trace2 reentrancy separately? I still can't explain why the\n>>>> allocation failed, so there may be another GfW-specific piece I’m\n>>>> missing.\n>>>\n>>> I think that your suggested change has merits and should be pursued.\n>>> I'll explore it a bit to confirm.\n>>\n>> That band-aid may be a good idea, but I would prefer not to see the\n>> conditional in a common source file like 'wrapper.c'. Somewhere\n>> MinGW-specific would be more appropriate, would it not?\n> \n> Did anything come out of this discussion?\n\nSorry that I've been unavailable to come back to this thread, but here\nis what I've learned in the meantime:\n\n* Taylor's hunch that the memory allocation is more likely at fault\n is seeming more and more correct. When we fixed this issue, other\n issues around memory allocation came to light.\n\n* For that reason, I'll rework this patch to point at the allocation\n as the likely reason the parsing fails. Avoiding a die() in the\n tracing code is still critical.\n\n* Thus, I'll also replace the xstrdup() in the trace code to avoid a\n die() due to allocation problems.\n\n* I will take a deeper look at this wrapper change and how it might\n be done in a careful way, as Taylor says his patch was an example\n only and not the \"right\" way to do it.\n\nThanks,\n-Stolee\n\n"},{"id":"549358","messageId":"xmqqpl03cfua.fsf@gitster.g","threadId":"66006","inReplyTo":"fbb118df-7c82-49a5-90bb-4458b7e9a850@gmail.com","subject":"Re: [PATCH] trace2: tolerate failed timestamp formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-31T15:57:01Z","receivedAt":"2026-07-31T15:57:04Z","isPatch":true,"body":"Derrick Stolee writes:\n\n> * Taylor's hunch that the memory allocation is more likely at fault\n> is seeming more and more correct. When we fixed this issue, other\n> issues around memory allocation came to light.\n>\n> * For that reason, I'll rework this patch to point at the allocation\n> as the likely reason the parsing fails. Avoiding a die() in the\n> tracing code is still critical.\n>\n> * Thus, I'll also replace the xstrdup() in the trace code to avoid a\n> die() due to allocation problems.\n>\n> * I will take a deeper look at this wrapper change and how it might\n> be done in a careful way, as Taylor says his patch was an example\n> only and not the \"right\" way to do it.\n\nThanks.\n"},{"id":"551217","messageId":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.git.1784131932489.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] trace2: stop allowing die()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:14Z","receivedAt":"2026-08-25T18:56:24Z","isPatch":true,"body":"After v1 was posted, based on a concrete example of tracing leading to a\nrecursive die() problem, more evidence has come up to imply that allocations\nare failing for some users more often. This is potentially an issue with the\nallocator chosen by Git for Windows, which is being discussed elsewhere.\n\nBut the conclusion is this: the trace2 API shouldn't call helpers that might\ncall die(). It's too low-level for that.\n\nIn this v2, I have a much more robust approach to removing die() from the\ntrace2 API.\n\nThis starts with a new banned-die.h header file at the root of the repo and\nincluding it from all trace2 API *.c files. It starts empty, but the later\npatches will add one method at a time:\n\n * xsnprintf() : This is the original patch, but made more complete by\n adding the method to banned-die.h.\n * xstrdup()\n * ALLOC_ARRAY()\n * xstrfmt()\n * ALLOC_GROW()\n * xcalloc()\n\nDuring each patch, the goal was to have the trace2 logic be \"as correct as\npossible\" when an allocation failure occurs. This may mean that we have\nincomplete messages or dropped trace messages.\n\nThe focus here is that the trace2 API should never cause a process-ending\nfailure, because those failures will trigger trace2 API calls while\nreporting the failure.\n\nThanks, -Stolee\n\nDerrick Stolee (7):\n banned-die: create header for banning of functions\n trace2: tolerate failed timestamp formatting\n trace2: remove use of xstrdup()\n trace2: remove use of ALLOC_ARRAY()\n trace2: remove use of xstrfmt()\n trace2: remove use of ALLOC_GROW()\n trace2: remove use of xcalloc()\n\n banned-die.h | 32 +++++++++++++++++\n trace2.c | 51 ++++++++++++++++++++++++---\n trace2/tr2_cfg.c | 1 +\n trace2/tr2_cmd_name.c | 1 +\n trace2/tr2_ctr.c | 11 +++++-\n trace2/tr2_dst.c | 1 +\n trace2/tr2_sid.c | 1 +\n trace2/tr2_sysenv.c | 7 ++--\n trace2/tr2_tbuf.c | 50 +++++++++++++++++++--------\n trace2/tr2_tgt_event.c | 1 +\n trace2/tr2_tgt_normal.c | 1 +\n trace2/tr2_tgt_perf.c | 1 +\n trace2/tr2_tls.c | 76 +++++++++++++++++++++++++++++++++++++++--\n trace2/tr2_tls.h | 7 ++++\n trace2/tr2_tmr.c | 15 ++++++--\n 15 files changed, 229 insertions(+), 27 deletions(-)\n create mode 100644 banned-die.h\n\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2178\n\nRange-diff vs v1:\n\n -: ---------- > 1: 84634717e2 banned-die: create header for banning of functions\n 1: 95c546bb3b ! 2: bd45f46a34 trace2: tolerate failed timestamp formatting\n @@ Commit message\n triggering this problem in a loop as the 'atexit' event would be\n retriggered by the die().\n \n - I could not determine the exact cause of why these errors started\n - occuring in a bunch. My best guess is that these users are dogfooding an\n - early operating system version that is more likely to fail in the\n - gettimeofday() function and thus leaves the structures uninitialized and\n - potentially violating the expected values.\n + Based on other symptoms impacting users on the version reporting these\n + failures, it is most likely that this is actually a failure to allocate\n + memory, which is a specific symptom in Git for Windows. That fork uses a\n + different library for its implementation of vsprintf() which allocates\n + an array when seven or more positional arguments exist in the formatting\n + string, such as this one.\n \n - However, for full defense-in-depth I made several modifications:\n + Ultimately, the trace2 machinery is so low-level that it should not rely on\n + any helper functions that perform error handling with die(), as that can\n + trigger issues that would then be traced, causing this kind of recursive\n + loop.\n +\n + These changes help remove any use of die() within this file:\n \n 1. Both 'tv' and 'tm' structs are initialized with zero values, allowing\n an erroring gettimeofday() or gmtime_r() method to leave them\n @@ Commit message\n but they only die() on out-of-memory errors instead of formatting\n issues. I chose to leave those in place for now.\n \n + Helped-by: Taylor Blau \n Signed-off-by: Derrick Stolee \n \n + ## banned-die.h ##\n +@@\n + #undef die\n + #define die banned(die)\n + \n ++#undef xsnprintf\n ++#define xsnprintf(...) BANNED(xsnprintf)\n ++\n + #endif /* BANNED_DIE_H */\n +\n ## trace2/tr2_tbuf.c ##\n @@\n \n -: ---------- > 3: ec447a6a77 trace2: remove use of xstrdup()\n -: ---------- > 4: db6858d381 trace2: remove use of ALLOC_ARRAY()\n -: ---------- > 5: 7f0bb405ad trace2: remove use of xstrfmt()\n -: ---------- > 6: 120cf1967b trace2: remove use of ALLOC_GROW()\n -: ---------- > 7: c8fc195a2a trace2: remove use of xcalloc()\n\n-- \ngitgitgadget\n"},{"id":"551219","messageId":"84634717e2eca479026d1cdf39a089a8f61d131e.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:15Z","receivedAt":"2026-08-25T18:56:27Z","isPatch":true,"body":"From: Derrick Stolee \n\nWe have universally-banned functions listed in banned.h since\nc8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\nthe code should be more strict than others.\n\nOne such example is the trace2 API which runs during atexit() and can\nprove to cause die()-handler recursion problems if it calls die().\n\nCreate a new banned-die.h header file that will ban some Git methods\nthat call die(). Include that in all trace2 API implementation files.\nThis currently only bans die() itself, and that was already not used.\n\nIt would be reasonable to name this file trace2/tr2_banned.h to be\nspecific to the trace2 API, but it seems like such a restriction would\nbe valuable to put in some other areas of the code, so adding it at the\nroot of the tree seems like a good long-term approach.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 14 ++++++++++++++\n trace2.c | 1 +\n trace2/tr2_cfg.c | 1 +\n trace2/tr2_cmd_name.c | 1 +\n trace2/tr2_ctr.c | 1 +\n trace2/tr2_dst.c | 1 +\n trace2/tr2_sid.c | 1 +\n trace2/tr2_sysenv.c | 1 +\n trace2/tr2_tbuf.c | 1 +\n trace2/tr2_tgt_event.c | 1 +\n trace2/tr2_tgt_normal.c | 1 +\n trace2/tr2_tgt_perf.c | 1 +\n trace2/tr2_tls.c | 1 +\n trace2/tr2_tmr.c | 1 +\n 14 files changed, 27 insertions(+)\n create mode 100644 banned-die.h\n\ndiff --git a/banned-die.h b/banned-die.h\nnew file mode 100644\nindex 0000000000..5eff361e55\n--- /dev/null\n+++ b/banned-die.h\n@@ -0,0 +1,14 @@\n+#ifndef BANNED_DIE_H\n+#define BANNED_DIE_H\n+\n+#include \"banned.h\"\n+\n+/*\n+ * This header lists functions that must not be used by low-level APIs\n+ * because they can cause Git to terminate.\n+ */\n+\n+#undef die\n+#define die banned(die)\n+\n+#endif /* BANNED_DIE_H */\ndiff --git a/trace2.c b/trace2.c\nindex c23c0a227b..1d0ed2db2b 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -17,6 +17,7 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+#include \"banned-die.h\"\n \n static int trace2_enabled;\n static int trace2_redact = 1;\ndiff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c\nindex bbcfeda60a..06912a3ceb 100644\n--- a/trace2/tr2_cfg.c\n+++ b/trace2/tr2_cfg.c\n@@ -7,6 +7,7 @@\n #include \"trace2/tr2_cfg.h\"\n #include \"trace2/tr2_sysenv.h\"\n #include \"wildmatch.h\"\n+#include \"banned-die.h\"\n \n static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;\n static int tr2_cfg_loaded;\ndiff --git a/trace2/tr2_cmd_name.c b/trace2/tr2_cmd_name.c\nindex b7b5a869b7..88f24e8781 100644\n--- a/trace2/tr2_cmd_name.c\n+++ b/trace2/tr2_cmd_name.c\n@@ -1,6 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n #include \"trace2/tr2_cmd_name.h\"\n+#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_NAME \"GIT_TRACE2_PARENT_NAME\"\n \ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex ee17bfa86b..3067df4d18 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -2,6 +2,7 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_ctr.h\"\n+#include \"banned-die.h\"\n \n /*\n * A global counter block to aggregate values from the partial sums\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex 5be892cd5c..686a3e42fc 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -5,6 +5,7 @@\n #include \"trace2/tr2_dst.h\"\n #include \"trace2/tr2_sid.h\"\n #include \"trace2/tr2_sysenv.h\"\n+#include \"banned-die.h\"\n \n /*\n * How many attempts we will make at creating an automatically-named trace file.\ndiff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c\nindex 1c1d27b0ee..358f61b301 100644\n--- a/trace2/tr2_sid.c\n+++ b/trace2/tr2_sid.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"trace2/tr2_tbuf.h\"\n #include \"trace2/tr2_sid.h\"\n+#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_SID \"GIT_TRACE2_PARENT_SID\"\n \ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex 4abc218514..deb3fabff4 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -4,6 +4,7 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"tr2_sysenv.h\"\n+#include \"banned-die.h\"\n \n /*\n * Each entry represents a trace2 setting.\ndiff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c\nindex c3b3822ed7..86725426f6 100644\n--- a/trace2/tr2_tbuf.c\n+++ b/trace2/tr2_tbuf.c\n@@ -1,5 +1,6 @@\n #include \"git-compat-util.h\"\n #include \"tr2_tbuf.h\"\n+#include \"banned-die.h\"\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n {\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 5a0381791f..a055e19bac 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -13,6 +13,7 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_event = {\n \t.sysenv_var = TR2_SYSENV_EVENT,\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex 924736ab36..97d4c5d202 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -11,6 +11,7 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_normal = {\n \t.sysenv_var = TR2_SYSENV_NORMAL,\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex 4eb9289f95..1f49d9f922 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -14,6 +14,7 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_perf = {\n \t.sysenv_var = TR2_SYSENV_PERF,\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 7b023c1bfc..ae2d39d2f5 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -3,6 +3,7 @@\n #include \"thread-utils.h\"\n #include \"trace.h\"\n #include \"trace2/tr2_tls.h\"\n+#include \"banned-die.h\"\n \n /*\n * Initialize size of the thread stack for nested regions.\ndiff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c\nindex 038181ad9b..a329c466b9 100644\n--- a/trace2/tr2_tmr.c\n+++ b/trace2/tr2_tmr.c\n@@ -3,6 +3,7 @@\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n #include \"trace.h\"\n+#include \"banned-die.h\"\n \n #define MY_MAX(a, b) ((a) > (b) ? (a) : (b))\n #define MY_MIN(a, b) ((a) < (b) ? (a) : (b))\n-- \ngitgitgadget\n\n"},{"id":"551218","messageId":"bd45f46a34aeb539d45d71b76c12c319abbfbcb5.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] trace2: tolerate failed timestamp formatting","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:16Z","receivedAt":"2026-08-25T18:56:28Z","isPatch":true,"body":"From: Derrick Stolee \n\nSome users reported issues of repeated messages:\n\n fatal: recursion detected in die handler\n\nThis wasn't happening every time, but we eventually captured a\nGIT_TRACE2_PERF log file with this issue and revealed an interesting\ninternal detail, failing with this message:\n\n unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\n\nThis specific format string tracks to tr2_tbuf_utc_datetime_extended()\nin trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but\nwas later split in bad229aef23 (trace2: clarify UTC datetime formatting,\n2019-04-15).\n\nThis use of xsnprintf() is writing a very specific datetime format into a\n32-character buffer. The format requires that the input data will not\noverflow the format digits or the buffer will not hold the result. Since\nwe are using xsnprintf() here, those failures turn into die() events.\n\nThis method and its siblings, tr2_tbuf_local_time() and\ntr2_tbuf_utc_datetime(), are used in the tracing library. The extended\nform is used only for the 'event' format, which these users were using\nvia a config setting for use in client-side telemetry. The non-extended\nform is used to help generate the 'SID' that defines the process in the\ntraces.\n\nNot only are these inappropriate times for a failure, but the extended\nmethod is called specifially during the 'atexit' event, which was\ntriggering this problem in a loop as the 'atexit' event would be\nretriggered by the die().\n\nBased on other symptoms impacting users on the version reporting these\nfailures, it is most likely that this is actually a failure to allocate\nmemory, which is a specific symptom in Git for Windows. That fork uses a\ndifferent library for its implementation of vsprintf() which allocates\nan array when seven or more positional arguments exist in the formatting\nstring, such as this one.\n\nUltimately, the trace2 machinery is so low-level that it should not rely on\nany helper functions that perform error handling with die(), as that can\ntrigger issues that would then be traced, causing this kind of recursive\nloop.\n\nThese changes help remove any use of die() within this file:\n\n1. Both 'tv' and 'tm' structs are initialized with zero values, allowing\n an erroring gettimeofday() or gmtime_r() method to leave them\n zero-valued. A zero-valued date is better than a die() here.\n\n2. Replace the use of xsnprintf() with snprintf() to avoid the\n possibility of calling die() here. Instead, check the response to see\n if there was a failure. On failure, put a blank value into the buffer\n instead of possibly allowing a value that would not format correctly\n for a trace2 consumer. This value should be seen as obviously wrong\n and therefore signals a problem.\n\nAs the core issue in this code seems to require a system method\nreturning an error, no test accompanies this change.\n\nThis change removes all uses of xsnprintf() from the trace2/ directory.\nThere are two uses of xstrdup() that could be considered for removal,\nbut they only die() on out-of-memory errors instead of formatting\nissues. I chose to leave those in place for now.\n\nHelped-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------\n 2 files changed, 37 insertions(+), 15 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 5eff361e55..0e0a794e5d 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -11,4 +11,7 @@\n #undef die\n #define die banned(die)\n \n+#undef xsnprintf\n+#define xsnprintf(...) BANNED(xsnprintf)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c\nindex 86725426f6..fff345cb99 100644\n--- a/trace2/tr2_tbuf.c\n+++ b/trace2/tr2_tbuf.c\n@@ -4,45 +4,64 @@\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tlocaltime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\", tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\",\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"00:00:00.000000\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf),\n-\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\", tm.tm_year + 1900,\n-\t\t tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,\n-\t\t (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"1900-00-00T00:00:00.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n-\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"19000000T000000.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n-- \ngitgitgadget\n\n"},{"id":"551220","messageId":"ec447a6a778a5c49344346df54b434a96c792082.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] trace2: remove use of xstrdup()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:17Z","receivedAt":"2026-08-25T18:56:29Z","isPatch":true,"body":"From: Derrick Stolee \n\nIn the previous change, we removed a use of xsprintf() that caused a\nrecursive die() loop when failing to allocate memory. The trace2 library is\ntoo low-level to be calling die(), especially because of these recursive\nloops that can occur during the die handler.\n\nFor full defense in depth, we remove the xstrdup() calls from\ntrace2/tr2_sysenv.c.\n\nFirst, in tr2_sysenv_cb(), we need to handle a failed assignment of the\nvalue with a negative return to halt the config parsing loop.\n\nSecond, in tr2_sysenv_get(), the method will return NULL when strdup()\nreturns NULL. This return is indistinguishable from the environment variable\nhaving no value. That means that all callers know how to handle a NULL\nresponse, but no behavior change will occur between the case of no\nenvironment being set and detecting an environment variable exists but we\nfail to duplicate it. This seems an appropriate trade-off, as an allocation\nfailure at this level will likely lead to failure in another system, but at\nleast the trace2 API will not cause the process to fail early.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_sysenv.c | 6 ++++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 0e0a794e5d..2e16c4899c 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -14,4 +14,7 @@\n #undef xsnprintf\n #define xsnprintf(...) BANNED(xsnprintf)\n \n+#undef xstrdup\n+#define xstrdup(str) BANNED(xstrdup)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex deb3fabff4..4ee273a4ae 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tfree(tr2_sysenv_settings[k].value);\n-\t\t\ttr2_sysenv_settings[k].value = xstrdup(value);\n+\t\t\ttr2_sysenv_settings[k].value = strdup(value);\n+\t\t\tif (!tr2_sysenv_settings[k].value)\n+\t\t\t\treturn -1;\n \t\t\treturn 0;\n \t\t}\n \t}\n@@ -110,7 +112,7 @@ const char *tr2_sysenv_get(enum tr2_sysenv_variable var)\n \t\tconst char *v = getenv(tr2_sysenv_settings[var].env_var_name);\n \t\tif (v && *v) {\n \t\t\tfree(tr2_sysenv_settings[var].value);\n-\t\t\ttr2_sysenv_settings[var].value = xstrdup(v);\n+\t\t\ttr2_sysenv_settings[var].value = strdup(v);\n \t\t}\n \t\ttr2_sysenv_settings[var].getenv_called = 1;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"551221","messageId":"db6858d3811c8cfdd136a0069f0ace33b95888ae.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] trace2: remove use of ALLOC_ARRAY()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:18Z","receivedAt":"2026-08-25T18:56:32Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe banned-die.h header is used to prevent use of helper methods that\ncall die(). Remove use of the ALLOC_ARRAY() helper, which calls die() on\nallocation failures. Replace the use in trace2.c with a more direct\nallocation and soft failure when allocation fails. This prevents die()\nrecursion loops when memory allocation fails and trace2 logs are\nenabled.\n\nThe tricky part about this change is how to handle the results from\nredact_arg(), which is a 'const char *' result because it might be a\npointer directly to the externally-controlled argument. When it is\ndifferent from the argument, then it is indeed a newly-allocated string\nthat we need to free before returning. This requires using a (char *)\ncast to allow a change.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2.c | 16 ++++++++++++++--\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 2e16c4899c..cb2eed75cd 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,4 +17,7 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef ALLOC_ARRAY\n+#define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2.c b/trace2.c\nindex 1d0ed2db2b..7044276435 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -304,7 +304,11 @@ static const char **redact_argv(const char **argv)\n \tfor (j = 0; argv[j]; j++)\n \t\t; /* keep counting */\n \n-\tALLOC_ARRAY(ret, j + 1);\n+\tret = calloc(j + 1, sizeof(*ret));\n+\tif (!ret) {\n+\t\tfree((char *)redacted);\n+\t\treturn NULL;\n+\t}\n \tret[j] = NULL;\n \n \tfor (j = 0; j < i; j++)\n@@ -345,6 +349,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n \tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n \n \tredacted = redact_argv(argv);\n+\tif (!redacted)\n+\t\treturn;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_start_fl)\n@@ -513,6 +519,7 @@ void trace2_child_start_fl(const char *file, int line,\n \tuint64_t us_now;\n \tuint64_t us_elapsed_absolute;\n \tconst char **orig_argv = cmd->args.v;\n+\tconst char **redacted;\n \n \tif (!trace2_enabled)\n \t\treturn;\n@@ -530,7 +537,10 @@ void trace2_child_start_fl(const char *file, int line,\n \t * temporarily replace the original argv (inside the `strvec`)\n \t * with a possibly redacted version.\n \t */\n-\tcmd->args.v = redact_argv(orig_argv);\n+\tredacted = redact_argv(orig_argv);\n+\tif (!redacted)\n+\t\treturn;\n+\tcmd->args.v = redacted;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_child_start_fl)\n@@ -622,6 +632,8 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n \texec_id = tr2tls_locked_increment(&tr2_next_exec_id);\n \n \tredacted = redact_argv(argv);\n+\tif (!redacted)\n+\t\treturn exec_id;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_exec_fl)\n-- \ngitgitgadget\n\n"},{"id":"551222","messageId":"7f0bb405ad380fd35ae6381961ac667fd7e5dfd9.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] trace2: remove use of xstrfmt()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:19Z","receivedAt":"2026-08-25T18:56:34Z","isPatch":true,"body":"From: Derrick Stolee \n\nWe continue removing the possibility of a die() in the trace2 API by\nbanning xstrfmt(), which calls die() during a failure to format. Instead\nof allowing a die(), perform a soft failure by failing to output the\ntrace2 data when such a failure occurs.\n\nThis requires carefully concatenating strings using memcpy() to\nconstruct redacted data to avoid copying password information in traced\nURLs.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2.c | 34 ++++++++++++++++++++++++++++++++--\n 2 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex cb2eed75cd..14aecfdc7a 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,6 +17,9 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef xstrfmt\n+#define xstrfmt(...) BANNED(xstrfmt)\n+\n #undef ALLOC_ARRAY\n #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n \ndiff --git a/trace2.c b/trace2.c\nindex 7044276435..c37f783fa0 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -260,7 +260,10 @@ int trace2_is_enabled(void)\n static const char *redact_arg(const char *arg)\n {\n \tconst char *p, *colon;\n+\tconst char *redact = \":\";\n+\tchar *redacted;\n \tsize_t at;\n+\tsize_t prefix_len, suffix_len, redacted_len, redact_len;\n \n \tif (!trace2_redact ||\n \t (!skip_prefix(arg, \"https://\", &p) &&\n@@ -275,7 +278,25 @@ static const char *redact_arg(const char *arg)\n \tif (!colon)\n \t\treturn arg;\n \n-\treturn xstrfmt(\"%.*s:%s\", (int)(colon - arg), arg, p + at);\n+\tredact_len = strlen(redact);\n+\tprefix_len = colon - arg;\n+\tsuffix_len = strlen(p + at);\n+\n+\tif (unsigned_add_overflows(prefix_len, suffix_len) ||\n+\t unsigned_add_overflows(prefix_len + suffix_len, redact_len))\n+\t\treturn NULL;\n+\n+\tredacted_len = prefix_len + suffix_len + redact_len;\n+\n+\tredacted = malloc(redacted_len);\n+\tif (!redacted)\n+\t\treturn NULL;\n+\n+\tmemcpy(redacted, arg, prefix_len);\n+\tmemcpy(redacted + prefix_len, redact, redact_len - 1);\n+\tmemcpy(redacted + prefix_len + redact_len - 1, p + at,\n+\t suffix_len + 1);\n+\treturn redacted;\n }\n \n /*\n@@ -300,6 +321,8 @@ static const char **redact_argv(const char **argv)\n \n \tif (!argv[i])\n \t\treturn argv;\n+\tif (!redacted)\n+\t\treturn NULL;\n \n \tfor (j = 0; argv[j]; j++)\n \t\t; /* keep counting */\n@@ -316,7 +339,14 @@ static const char **redact_argv(const char **argv)\n \tret[i] = redacted;\n \tfor (++i; argv[i]; i++) {\n \t\tredacted = redact_arg(argv[i]);\n-\t\tret[i] = redacted ? redacted : argv[i];\n+\t\tif (!redacted) {\n+\t\t\tfor (j = 0; j < i; j++)\n+\t\t\t\tif (ret[j] != argv[j])\n+\t\t\t\t\tfree((void *)ret[j]);\n+\t\t\tfree(ret);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tret[i] = redacted;\n \t}\n \n \treturn ret;\n-- \ngitgitgadget\n\n"},{"id":"551223","messageId":"120cf1967bde4e719a781c391b285c718553ad58.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] trace2: remove use of ALLOC_GROW()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:20Z","receivedAt":"2026-08-25T18:56:36Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe ALLOC_GROW() helper can call die() on a failed memory allocation.\nWe need to remove this from the trace2 API code to prevent a recursive\ndie() handler.\n\nThis helper is used to track the nested region stack. Use a new\nskipped_regions member to track how many times a region was entered\nwithout being added to the stack, and decrease that amount as we leave\neach region. This allows us to avoid a failure and instead stop\ndeepening the stack, giving as much nesting behavior as possible without\nfailing the entire process.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_tls.c | 34 +++++++++++++++++++++++++++++++++-\n trace2/tr2_tls.h | 1 +\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 14aecfdc7a..423e7b607d 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -23,4 +23,7 @@\n #undef ALLOC_ARRAY\n #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n \n+#undef ALLOC_GROW\n+#define ALLOC_GROW(x, nr, alloc) BANNED(ALLOC_GROW)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex ae2d39d2f5..8596292a94 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -108,8 +108,33 @@ void tr2tls_unset_self(void)\n void tr2tls_push_self(uint64_t us_now)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n+\tuint64_t *new_array;\n+\tsize_t new_alloc;\n+\n+\tif (ctx->nr_skipped_regions) {\n+\t\tctx->nr_skipped_regions++;\n+\t\treturn;\n+\t}\n+\n+\tif (ctx->nr_open_regions < ctx->alloc)\n+\t\treturn;\n+\n+\tif (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n+\t\tctx->nr_skipped_regions++;\n+\t\treturn;\n+\t}\n+\tnew_alloc = ctx->alloc * 2;\n+\n+\tnew_array = realloc(ctx->array_us_start,\n+\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n+\tif (!new_array) {\n+\t\tctx->nr_skipped_regions++;\n+\t\treturn;\n+\t}\n+\n+\tctx->array_us_start = new_array;\n+\tctx->alloc = new_alloc;\n \n-\tALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc);\n \tctx->array_us_start[ctx->nr_open_regions++] = us_now;\n }\n \n@@ -117,6 +142,11 @@ void tr2tls_pop_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (ctx->nr_skipped_regions) {\n+\t\tctx->nr_skipped_regions--;\n+\t\treturn;\n+\t}\n+\n \tif (!ctx->nr_open_regions)\n \t\tBUG(\"no open regions in thread '%s'\", ctx->thread_name);\n \n@@ -137,6 +167,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)\n \tuint64_t us_start;\n \n \tctx = tr2tls_get_self();\n+\tif (ctx->nr_skipped_regions)\n+\t\treturn 0;\n \tif (!ctx->nr_open_regions)\n \t\treturn 0;\n \ndiff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h\nindex 3bdbf4d275..c365017923 100644\n--- a/trace2/tr2_tls.h\n+++ b/trace2/tr2_tls.h\n@@ -20,6 +20,7 @@ struct tr2tls_thread_ctx {\n \tuint64_t *array_us_start;\n \tsize_t alloc;\n \tsize_t nr_open_regions; /* plays role of \"nr\" in ALLOC_GROW */\n+\tsize_t nr_skipped_regions;\n \tint thread_id;\n \tstruct tr2_timer_block timer_block;\n \tstruct tr2_counter_block counter_block;\n-- \ngitgitgadget\n\n"},{"id":"551224","messageId":"c8fc195a2ace4c2058ffa87e40a5745d349ab2dc.1787684181.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] trace2: remove use of xcalloc()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-25T18:56:21Z","receivedAt":"2026-08-25T18:56:36Z","isPatch":true,"body":"From: Derrick Stolee \n\nRemove use of xcalloc() from the trace2 API due to its possible use of\ndie(), which could lead to recursive die() handlers. This is used in the\ntrace2 API to track an array of thread contexts when logging multi-\nthreaded operations.\n\nInstead of killing the process on a failure, we attempt to proceed as\nmuch as possible. We replace the dynamic thread context with a\nstatically-allocated context that uses the \"unknown\" thread name to\nidentify that we are in an error case.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 ++\n trace2/tr2_ctr.c | 10 ++++++-\n trace2/tr2_tls.c | 73 ++++++++++++++++++++++++++++++++++++------------\n trace2/tr2_tls.h | 6 ++++\n trace2/tr2_tmr.c | 14 ++++++++--\n 5 files changed, 85 insertions(+), 21 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 423e7b607d..3dc521f6b0 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,6 +17,9 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef xcalloc\n+#define xcalloc(nmemb, size) BANNED(xcalloc)\n+\n #undef xstrfmt\n #define xstrfmt(...) BANNED(xstrfmt)\n \ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex 3067df4d18..5283946e08 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -54,7 +54,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n void tr2_counter_increment(enum trace2_counter_id cid, uint64_t value)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_counter *c = &ctx->counter_block.counter[cid];\n+\tstruct tr2_counter *c;\n+\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tc = &ctx->counter_block.counter[cid];\n \n \tc->value += value;\n \n@@ -68,6 +72,8 @@ void tr2_update_final_counters(void)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_counter_id cid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_counter)\n \t\treturn;\n \n@@ -89,6 +95,8 @@ void tr2_emit_per_thread_counters(tr2_tgt_evt_counter_t *fn_apply)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_counter_id cid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_per_thread_counter)\n \t\treturn;\n \ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 8596292a94..2c6aaed504 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -13,6 +13,9 @@\n #define TR2_REGION_NESTING_INITIAL_SIZE (100)\n \n static struct tr2tls_thread_ctx *tr2tls_thread_main;\n+static struct tr2tls_thread_ctx tr2tls_thread_fallback = {\n+\t.thread_name = \"unknown\",\n+};\n static uint64_t tr2tls_us_start_process;\n \n static pthread_mutex_t tr2tls_mutex;\n@@ -37,16 +40,23 @@ void tr2tls_start_process_clock(void)\n struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n \t\t\t\t\t uint64_t us_thread_start)\n {\n-\tstruct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));\n+\tstruct tr2tls_thread_ctx *ctx = calloc(1, sizeof(*ctx));\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!ctx)\n+\t\tgoto fallback;\n+\n \t/*\n \t * Implicitly \"tr2tls_push_self()\" to capture the thread's start\n \t * time in array_us_start[0]. For the main thread this gives us the\n \t * application run time.\n \t */\n \tctx->alloc = TR2_REGION_NESTING_INITIAL_SIZE;\n-\tctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t));\n+\tctx->array_us_start = calloc(ctx->alloc, sizeof(uint64_t));\n+\tif (!ctx->array_us_start) {\n+\t\tfree(ctx);\n+\t\tgoto fallback;\n+\t}\n \tctx->array_us_start[ctx->nr_open_regions++] = us_thread_start;\n \n \tctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);\n@@ -62,6 +72,10 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n \tpthread_setspecific(tr2tls_key, ctx);\n \n \treturn ctx;\n+\n+fallback:\n+\tpthread_setspecific(tr2tls_key, &tr2tls_thread_fallback);\n+\treturn &tr2tls_thread_fallback;\n }\n \n struct tr2tls_thread_ctx *tr2tls_get_self(void)\n@@ -84,6 +98,11 @@ struct tr2tls_thread_ctx *tr2tls_get_self(void)\n \treturn ctx;\n }\n \n+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx)\n+{\n+\treturn ctx == &tr2tls_thread_fallback;\n+}\n+\n int tr2tls_is_main_thread(void)\n {\n \tif (!HAVE_THREADS)\n@@ -100,6 +119,9 @@ void tr2tls_unset_self(void)\n \n \tpthread_setspecific(tr2tls_key, NULL);\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tfree((char *)ctx->thread_name);\n \tfree(ctx->array_us_start);\n \tfree(ctx);\n@@ -111,30 +133,33 @@ void tr2tls_push_self(uint64_t us_now)\n \tuint64_t *new_array;\n \tsize_t new_alloc;\n \n-\tif (ctx->nr_skipped_regions) {\n-\t\tctx->nr_skipped_regions++;\n-\t\treturn;\n-\t}\n-\n-\tif (ctx->nr_open_regions < ctx->alloc)\n+\tif (tr2tls_is_fallback(ctx))\n \t\treturn;\n \n-\tif (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n+\tif (ctx->nr_skipped_regions) {\n \t\tctx->nr_skipped_regions++;\n \t\treturn;\n \t}\n-\tnew_alloc = ctx->alloc * 2;\n \n-\tnew_array = realloc(ctx->array_us_start,\n-\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n-\tif (!new_array) {\n-\t\tctx->nr_skipped_regions++;\n-\t\treturn;\n+\tif (ctx->nr_open_regions >= ctx->alloc) {\n+\t\tif (ctx->alloc >\n+\t\t SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n+\t\t\tctx->nr_skipped_regions++;\n+\t\t\treturn;\n+\t\t}\n+\t\tnew_alloc = ctx->alloc * 2;\n+\n+\t\tnew_array = realloc(ctx->array_us_start,\n+\t\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n+\t\tif (!new_array) {\n+\t\t\tctx->nr_skipped_regions++;\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tctx->array_us_start = new_array;\n+\t\tctx->alloc = new_alloc;\n \t}\n \n-\tctx->array_us_start = new_array;\n-\tctx->alloc = new_alloc;\n-\n \tctx->array_us_start[ctx->nr_open_regions++] = us_now;\n }\n \n@@ -142,6 +167,9 @@ void tr2tls_pop_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tif (ctx->nr_skipped_regions) {\n \t\tctx->nr_skipped_regions--;\n \t\treturn;\n@@ -157,6 +185,9 @@ void tr2tls_pop_unwind_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \twhile (ctx->nr_open_regions > 1)\n \t\ttr2tls_pop_self();\n }\n@@ -167,6 +198,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)\n \tuint64_t us_start;\n \n \tctx = tr2tls_get_self();\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn 0;\n \tif (ctx->nr_skipped_regions)\n \t\treturn 0;\n \tif (!ctx->nr_open_regions)\n@@ -188,6 +221,10 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)\n static void tr2tls_key_destructor(void *payload)\n {\n \tstruct tr2tls_thread_ctx *ctx = payload;\n+\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tfree((char *)ctx->thread_name);\n \tfree(ctx->array_us_start);\n \tfree(ctx);\ndiff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h\nindex c365017923..4a0969c014 100644\n--- a/trace2/tr2_tls.h\n+++ b/trace2/tr2_tls.h\n@@ -54,6 +54,12 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n */\n struct tr2tls_thread_ctx *tr2tls_get_self(void);\n \n+/*\n+ * Return true if the context is the non-allocating fallback used after an\n+ * allocation failure. Callers must not modify a fallback context.\n+ */\n+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx);\n+\n /*\n * return true if the current thread is the main thread.\n */\ndiff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c\nindex a329c466b9..b3d26e2b31 100644\n--- a/trace2/tr2_tmr.c\n+++ b/trace2/tr2_tmr.c\n@@ -38,8 +38,11 @@ static struct tr2_timer_metadata tr2_timer_metadata[TRACE2_NUMBER_OF_TIMERS] = {\n void tr2_start_timer(enum trace2_timer_id tid)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_timer *t = &ctx->timer_block.timer[tid];\n+\tstruct tr2_timer *t;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tt = &ctx->timer_block.timer[tid];\n \tt->recursion_count++;\n \tif (t->recursion_count > 1)\n \t\treturn; /* ignore recursive starts */\n@@ -50,10 +53,13 @@ void tr2_start_timer(enum trace2_timer_id tid)\n void tr2_stop_timer(enum trace2_timer_id tid)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_timer *t = &ctx->timer_block.timer[tid];\n+\tstruct tr2_timer *t;\n \tuint64_t ns_now;\n \tuint64_t ns_interval;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tt = &ctx->timer_block.timer[tid];\n \tassert(t->recursion_count > 0);\n \n \tt->recursion_count--;\n@@ -91,6 +97,8 @@ void tr2_update_final_timers(void)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_timer_id tid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_timer)\n \t\treturn;\n \n@@ -137,6 +145,8 @@ void tr2_emit_per_thread_timers(tr2_tgt_evt_timer_t *fn_apply)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_timer_id tid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_per_thread_timer)\n \t\treturn;\n \n-- \ngitgitgadget\n"},{"id":"551237","messageId":"xmqqh5kikkgi.fsf@gitster.g","threadId":"66006","inReplyTo":"84634717e2eca479026d1cdf39a089a8f61d131e.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-25T20:34:53Z","receivedAt":"2026-08-25T20:34:56Z","isPatch":true,"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> We have universally-banned functions listed in banned.h since\n> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\n> the code should be more strict than others.\n>\n> One such example is the trace2 API which runs during atexit() and can\n> prove to cause die()-handler recursion problems if it calls die().\n>\n> Create a new banned-die.h header file that will ban some Git methods\n> that call die(). Include that in all trace2 API implementation files.\n> This currently only bans die() itself, and that was already not used.\n>\n> It would be reasonable to name this file trace2/tr2_banned.h to be\n> specific to the trace2 API, but it seems like such a restriction would\n> be valuable to put in some other areas of the code, so adding it at the\n> root of the tree seems like a good long-term approach.\n\nIn other words, the functions banned by including this file are not\nlisted because they are banned from being used in trace2 API, but\nbecause they may lead to die(). There may be some other traits that\nwe might want to avoid in certain subset of our code, and we may\nhave similar banned-frotz.h header to prevent direct or indirect use\nof frotz. Which makes sense to me.\n\nWould the same approach work for the_hash_algo and the_repository, I\nwonder?\n\n"},{"id":"551254","messageId":"CABPp-BHtmjSqkgL+RL=nmd1VNqqZ6vDUhQxj0AnEzAHZxznoHw@mail.gmail.com","threadId":"66006","inReplyTo":"84634717e2eca479026d1cdf39a089a8f61d131e.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T22:14:26Z","receivedAt":"2026-08-25T22:14:38Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n[...]\n> +#undef die\n> +#define die banned(die)\n\nShouldn't that be BANNED(die) to match all the other cases in the code\n(and avoid an obtuse \"implicit declaration of function 'banned'\"\ninstead of the nicer \"sorry_die_is_a_banned_function\" message)?\n\n> +\n> +#endif /* BANNED_DIE_H */\n> diff --git a/trace2.c b/trace2.c\n> index c23c0a227b..1d0ed2db2b 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -17,6 +17,7 @@\n> #include \"trace2/tr2_tgt.h\"\n> #include \"trace2/tr2_tls.h\"\n> #include \"trace2/tr2_tmr.h\"\n> +#include \"banned-die.h\"\n>\n\nIs there a risk that future folks add new includes at the end of the\nlist, then functions in them get added to banned-die.h, but are\nsilently ignored because banned-die.h wasn't the last include?\n"},{"id":"551255","messageId":"CABPp-BH1TeDTeqddZw+cvzou+3PRgw+HNpYF2JnhMTSBp9qfbQ@mail.gmail.com","threadId":"66006","inReplyTo":"ec447a6a778a5c49344346df54b434a96c792082.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/7] trace2: remove use of xstrdup()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T22:14:37Z","receivedAt":"2026-08-25T22:14:50Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n[...]\n> For full defense in depth, we remove the xstrdup() calls from\n> trace2/tr2_sysenv.c.\n>\n> First, in tr2_sysenv_cb(), we need to handle a failed assignment of the\n> value with a negative return to halt the config parsing loop.\n>\n[...]\n> --- a/trace2/tr2_sysenv.c\n> +++ b/trace2/tr2_sysenv.c\n> @@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,\n> if (!value)\n> return config_error_nonbool(key);\n> free(tr2_sysenv_settings[k].value);\n> - tr2_sysenv_settings[k].value = xstrdup(value);\n> + tr2_sysenv_settings[k].value = strdup(value);\n> + if (!tr2_sysenv_settings[k].value)\n> + return -1;\n\nI'm not sure if this matters, but I think the call sequence from\nconfig.c to this function is:\n\n read_very_early_config ->\n config_with_options ->\n git_config_from_file_with_options ->\n do_config_from_file ->\n do_config_from ->\n git_parse_source ->\n get_value ->\n git_config_include ->\n tr2_sysenv_cb\n\nand the -1 unwinds back to git_parse_source, which breaks, formats an\nerror message, and calls die:\n\n error_msg = xstrfmt(_(\"bad config line %d in file %s\")...)\n die(\"%s\", error_msg)\n\nAm I reading this right? If so, the -1 actually triggers a die as\nwell -- unless the allocation in xstrfmt manages to kill it first.\nThis isn't a regression (the old xstrdup() also died) and the die\nisn't inside the trace functions, but the commit message might read as\npromising more than it delivers.\n"},{"id":"551256","messageId":"CABPp-BHxpt1UBTY5LCn9OFMZ6EtOcUPc-61RMWvjpjDBmv1rzg@mail.gmail.com","threadId":"66006","inReplyTo":"7f0bb405ad380fd35ae6381961ac667fd7e5dfd9.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/7] trace2: remove use of xstrfmt()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T22:14:49Z","receivedAt":"2026-08-25T22:15:02Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 11:59 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n[...]\n>+ const char *redact = \":\";\n>+ char *redacted;\n[...]\n> + memcpy(redacted, arg, prefix_len);\n> + memcpy(redacted + prefix_len, redact, redact_len - 1);\n\nOnly copy redact_len - 1 bytes? So only \":\" ? Why?\n\n\n> + memcpy(redacted + prefix_len + redact_len - 1, p + at,\n> + suffix_len + 1);\n> + return redacted;\n> }\n>\n"},{"id":"551257","messageId":"CABPp-BFsSPJutOKManq-55ri=ddBpWLfN16xSNVs9O7+c2z0cg@mail.gmail.com","threadId":"66006","inReplyTo":"120cf1967bde4e719a781c391b285c718553ad58.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 6/7] trace2: remove use of ALLOC_GROW()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-08-25T22:14:55Z","receivedAt":"2026-08-25T22:15:08Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 11:57 AM Derrick Stolee via GitGitGadget\n wrote:\n>\n> From: Derrick Stolee \n>\n> The ALLOC_GROW() helper can call die() on a failed memory allocation.\n> We need to remove this from the trace2 API code to prevent a recursive\n> die() handler.\n>\n> This helper is used to track the nested region stack. Use a new\n> skipped_regions member to track how many times a region was entered\n> without being added to the stack, and decrease that amount as we leave\n> each region. This allows us to avoid a failure and instead stop\n> deepening the stack, giving as much nesting behavior as possible without\n> failing the entire process.\n>\n> Signed-off-by: Derrick Stolee \n\nChecking out this commit and running\n\n GIT_TRACE2_PERF=1 ./bin-wrappers/git status\n\ndies with\n\n no open regions in thread 'main'\n\nSeems to be fixed by 7/7, though. Maybe a bad splitting?\n"},{"id":"551258","messageId":"xmqqy0dtket5.fsf@gitster.g","threadId":"66006","inReplyTo":"CABPp-BHxpt1UBTY5LCn9OFMZ6EtOcUPc-61RMWvjpjDBmv1rzg@mail.gmail.com","subject":"Re: [PATCH v2 5/7] trace2: remove use of xstrfmt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-25T22:36:54Z","receivedAt":"2026-08-25T22:36:56Z","isPatch":true,"body":"Elijah Newren writes:\n\n> On Tue, Aug 25, 2026 at 11:59 AM Derrick Stolee via GitGitGadget\n> wrote:\n>>\n> [...]\n>>+ const char *redact = \":\";\n>>+ char *redacted;\n> [...]\n>> + memcpy(redacted, arg, prefix_len);\n>> + memcpy(redacted + prefix_len, redact, redact_len - 1);\n>\n> Only copy redact_len - 1 bytes? So only \": trailing \">\" ? Why?\n\nYeah, if it were (redact_len + 1) it would have worked better, perhaps?\n\n>\n>\n>> + memcpy(redacted + prefix_len + redact_len - 1, p + at,\n>> + suffix_len + 1);\n>> + return redacted;\n>> }\n>>\n"},{"id":"551337","messageId":"20260827051053.GB176544@coredump.intra.peff.net","threadId":"66006","inReplyTo":"84634717e2eca479026d1cdf39a089a8f61d131e.1787684181.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-27T05:10:53Z","receivedAt":"2026-08-27T05:10:55Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 06:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> We have universally-banned functions listed in banned.h since\n> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\n> the code should be more strict than others.\n> \n> One such example is the trace2 API which runs during atexit() and can\n> prove to cause die()-handler recursion problems if it calls die().\n> \n> Create a new banned-die.h header file that will ban some Git methods\n> that call die(). Include that in all trace2 API implementation files.\n> This currently only bans die() itself, and that was already not used.\n\nThere's a subtle but big difference between the universal code bans in\nbanned.h and this banned-die.h. In the former case we are deciding\nstrcpy() is unfit for our code base and outlawing it everywhere. The\npotential problem is in the source code, so catching it while compiling\nthe source code is OK.\n\nBut we are not doing that with die(). It is a perfectly OK function in\ngeneral, but we do not want to ever trigger its runtime effects from\ncertain code paths. Banning it from being called from those code paths\ncan catch _some_ instances, but not any transitive calls. If we call\nfoo(), it may call die() itself, and we would not want to ban foo() from\ndoing so. And recursively for functions called by foo() and so on.\n\nSo you end up playing whack-a-mole with functions that might call die()\nand adding them to this ban list.\n\nI think that's _probably_ the best we can do in practice. I think the\nframing above suggests that we could approach the problem more directly\nwith a runtime flag: when we enter those code paths, set a flag to avoid\nthe unwanted behavior, and have the low-level code respect that. But\ndie() is a special case here, because we'd want to suppress its\nno-return behavior. And its callers are not prepared for die() to\nsuddenly start returning because of some global flag.\n\nSo I think the whack-a-mole is the best we can do. But I would not want\nto see this strategy extended to other areas. In most cases some kind of\nruntime support is probably a better solution.\n\n-Peff\n"},{"id":"551339","messageId":"20260827052318.GC176544@coredump.intra.peff.net","threadId":"66006","inReplyTo":"pull.2178.v2.git.1787684181.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] trace2: stop allowing die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-27T05:23:18Z","receivedAt":"2026-08-27T05:23:20Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 06:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> This starts with a new banned-die.h header file at the root of the repo and\n> including it from all trace2 API *.c files. It starts empty, but the later\n> patches will add one method at a time:\n> \n> * xsnprintf() : This is the original patch, but made more complete by\n> adding the method to banned-die.h.\n> * xstrdup()\n> * ALLOC_ARRAY()\n> * xstrfmt()\n> * ALLOC_GROW()\n> * xcalloc()\n\nOK. This feels like the tip of the iceberg, though. All of strbuf would\nhave to be off-limits, too (both because it calls malloc directly, but\nalso because it will bail if snprintf() returns -1). I won't be\nsurprised if there are other indirect calls hiding in various places\n(e.g., all of json-writer.c).\n\nI think if you really want to avoid allocations in trace2 it would\nprobably need to be a ground-up no-dependency rewrite.\n\n-Peff\n"},{"id":"551542","messageId":"202e5eac-becb-4b75-80ca-5d56caf36f3a@gmail.com","threadId":"66006","inReplyTo":"xmqqh5kikkgi.fsf@gitster.g","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T12:28:39Z","receivedAt":"2026-08-31T12:28:42Z","isPatch":true,"body":"On 8/25/2026 4:34 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n\n>> It would be reasonable to name this file trace2/tr2_banned.h to be\n>> specific to the trace2 API, but it seems like such a restriction would\n>> be valuable to put in some other areas of the code, so adding it at the\n>> root of the tree seems like a good long-term approach.\n> \n> In other words, the functions banned by including this file are not\n> listed because they are banned from being used in trace2 API, but\n> because they may lead to die(). There may be some other traits that\n> we might want to avoid in certain subset of our code, and we may\n> have similar banned-frotz.h header to prevent direct or indirect use\n> of frotz. Which makes sense to me.\n> \n> Would the same approach work for the_hash_algo and the_repository, I\n> wonder?\n\nI'd be curious if it would satisfy two directions for those cases:\n\n1. Help declare a subsystem is free of these globals and thus is\n ready for multi-hash or multi-repo handling.\n\n2. Help declare a subsystem is _not_ free of these globals and thus\n should not be _reintroduced_ into a subsystem that was declared\n clean.\n\nWe'd need both, in general. And we'd need to continue expanding the\nbanned-*.h files. I am curious as to whether there are static tools\nthat could assist with this.\n\nThanks,\n-Stolee\n\n\n"},{"id":"551543","messageId":"4f9348a8-b71a-4583-8451-99ade6fced89@gmail.com","threadId":"66006","inReplyTo":"CABPp-BHtmjSqkgL+RL=nmd1VNqqZ6vDUhQxj0AnEzAHZxznoHw@mail.gmail.com","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T12:29:51Z","receivedAt":"2026-08-31T12:29:54Z","isPatch":true,"body":"On 8/25/2026 6:14 PM, Elijah Newren wrote:\n> On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget\n> wrote:\n>>\n> [...]\n>> +#undef die\n>> +#define die banned(die)\n> \n> Shouldn't that be BANNED(die) to match all the other cases in the code\n> (and avoid an obtuse \"implicit declaration of function 'banned'\"\n> instead of the nicer \"sorry_die_is_a_banned_function\" message)?\n\nOops. Yes, a mistake during a rebase. \n>> +\n>> +#endif /* BANNED_DIE_H */\n>> diff --git a/trace2.c b/trace2.c\n>> index c23c0a227b..1d0ed2db2b 100644\n>> --- a/trace2.c\n>> +++ b/trace2.c\n>> @@ -17,6 +17,7 @@\n>> #include \"trace2/tr2_tgt.h\"\n>> #include \"trace2/tr2_tls.h\"\n>> #include \"trace2/tr2_tmr.h\"\n>> +#include \"banned-die.h\"\n>>\n> \n> Is there a risk that future folks add new includes at the end of the\n> list, then functions in them get added to banned-die.h, but are\n> silently ignored because banned-die.h wasn't the last include?\n\nThere is a risk. The \"must be last\" part is documented in the\nheader, but maybe it should be in a comment here, too.\n\nThanks,\n-Stolee\n\n"},{"id":"551544","messageId":"9a84da3f-3409-49ea-be57-1e17e373273c@gmail.com","threadId":"66006","inReplyTo":"20260827051053.GB176544@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T12:38:01Z","receivedAt":"2026-08-31T12:38:06Z","isPatch":true,"body":"On 8/27/2026 1:10 AM, Jeff King wrote:\n> On Tue, Aug 25, 2026 at 06:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> We have universally-banned functions listed in banned.h since\n>> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\n>> the code should be more strict than others.\n>>\n>> One such example is the trace2 API which runs during atexit() and can\n>> prove to cause die()-handler recursion problems if it calls die().\n>>\n>> Create a new banned-die.h header file that will ban some Git methods\n>> that call die(). Include that in all trace2 API implementation files.\n>> This currently only bans die() itself, and that was already not used.\n> \n> There's a subtle but big difference between the universal code bans in\n> banned.h and this banned-die.h. In the former case we are deciding\n> strcpy() is unfit for our code base and outlawing it everywhere. The\n> potential problem is in the source code, so catching it while compiling\n> the source code is OK.\n> \n> But we are not doing that with die(). It is a perfectly OK function in\n> general, but we do not want to ever trigger its runtime effects from\n> certain code paths. Banning it from being called from those code paths\n> can catch _some_ instances, but not any transitive calls. If we call\n> foo(), it may call die() itself, and we would not want to ban foo() from\n> doing so. And recursively for functions called by foo() and so on.\n\nYes, this makes it tricky to be 100% sure without some kind of static\nanalysis.\n\n> So you end up playing whack-a-mole with functions that might call die()\n> and adding them to this ban list.\n\nThis does have some benefit that we can gradually remove these\ntransitive callers in the multi-commit series. But it's unsatisfying\nas a full protection in the end.\n\n> I think that's _probably_ the best we can do in practice. I think the\n> framing above suggests that we could approach the problem more directly\n> with a runtime flag: when we enter those code paths, set a flag to avoid\n> the unwanted behavior, and have the low-level code respect that. But\n> die() is a special case here, because we'd want to suppress its\n> no-return behavior. And its callers are not prepared for die() to\n> suddenly start returning because of some global flag.\n> \n> So I think the whack-a-mole is the best we can do. But I would not want\n> to see this strategy extended to other areas. In most cases some kind of\n> runtime support is probably a better solution.\nThe other alternative that we could consider is to reorganize the\ncodebase in such a way that certain sections of code don't have\naccess to headers that could lead to die() or other \"higher\" methods\nthat are acceptable for user-facing processes but are best to avoid\nin library APIs. Even then, we'd need some checks at compile time to\navoid crossing boundaries.\n\nI don't think such a reorganization is desirable overall, because\nthat will be very disruptive to the project and file history.\n\nHaving some amount of protection through this header gives us a\nmechanism to demonstrate and enforce some protection.\n\nThanks,\n-Stolee\n\n"},{"id":"551545","messageId":"2eadc838-9d47-442d-a94a-efc570624489@gmail.com","threadId":"66006","inReplyTo":"CABPp-BH1TeDTeqddZw+cvzou+3PRgw+HNpYF2JnhMTSBp9qfbQ@mail.gmail.com","subject":"Re: [PATCH v2 3/7] trace2: remove use of xstrdup()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T12:41:44Z","receivedAt":"2026-08-31T12:41:47Z","isPatch":true,"body":"On 8/25/2026 6:14 PM, Elijah Newren wrote:\n> On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget\n> wrote:\n>>\n> [...]\n>> For full defense in depth, we remove the xstrdup() calls from\n>> trace2/tr2_sysenv.c.\n>>\n>> First, in tr2_sysenv_cb(), we need to handle a failed assignment of the\n>> value with a negative return to halt the config parsing loop.\n>>\n> [...]\n>> --- a/trace2/tr2_sysenv.c\n>> +++ b/trace2/tr2_sysenv.c\n>> @@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,\n>> if (!value)\n>> return config_error_nonbool(key);\n>> free(tr2_sysenv_settings[k].value);\n>> - tr2_sysenv_settings[k].value = xstrdup(value);\n>> + tr2_sysenv_settings[k].value = strdup(value);\n>> + if (!tr2_sysenv_settings[k].value)\n>> + return -1;\n> \n> I'm not sure if this matters, but I think the call sequence from\n> config.c to this function is:\n> \n> read_very_early_config ->\n> config_with_options ->\n> git_config_from_file_with_options ->\n> do_config_from_file ->\n> do_config_from ->\n> git_parse_source ->\n> get_value ->\n> git_config_include ->\n> tr2_sysenv_cb\n> \n> and the -1 unwinds back to git_parse_source, which breaks, formats an\n> error message, and calls die:\n> \n> error_msg = xstrfmt(_(\"bad config line %d in file %s\")...)\n> die(\"%s\", error_msg)\n\nThanks for the careful read! It's particularly important that we\ndon't suggest that the config value is bad because we couldn't\nallocate memory.\n\n> Am I reading this right? If so, the -1 actually triggers a die as\n> well -- unless the allocation in xstrfmt manages to kill it first.\n> This isn't a regression (the old xstrdup() also died) and the die\n> isn't inside the trace functions, but the commit message might read as\n> promising more than it delivers.\n\nYes, I believe you are correct. We should return 0 to terminate\nearly without a failure.\n\nThat said, I think that the die() in the config code will remain a\n\"safe\" place to die(), as we won't re-trigger this config-parsing\ncode during any tracing of that die() message. But it's best to be\nsafe and have the tracing continue to be \"best effort\" when system\ncalls fail.\n\nThanks,\n-Stolee\n\n"},{"id":"551546","messageId":"feddbfb8-9b1d-4bfd-980a-9d51e05ee0ee@gmail.com","threadId":"66006","inReplyTo":"xmqqy0dtket5.fsf@gitster.g","subject":"Re: [PATCH v2 5/7] trace2: remove use of xstrfmt()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T12:51:00Z","receivedAt":"2026-08-31T12:51:06Z","isPatch":true,"body":"On 8/25/2026 6:36 PM, Junio C Hamano wrote:\n> Elijah Newren writes:\n> \n>> On Tue, Aug 25, 2026 at 11:59 AM Derrick Stolee via GitGitGadget\n>> wrote:\n>>>\n>> [...]\n>>> + const char *redact = \":\";\n>>> + char *redacted;\n>> [...]\n>>> + memcpy(redacted, arg, prefix_len);\n>>> + memcpy(redacted + prefix_len, redact, redact_len - 1);\n>>\n>> Only copy redact_len - 1 bytes? So only \":> trailing \">\" ? Why?\n> \n> Yeah, if it were (redact_len + 1) it would have worked better, perhaps?\nI should have been more careful and realized that we don't have any\ntests that cover this logic.\n\nWe have tests for \":\" in pkt-line output, but not for the\ntrace2 version.\n\nThanks,\n-Stolee\n\n"},{"id":"551554","messageId":"a41bdb3b-1fe7-4c1e-9d16-72390d93503b@gmail.com","threadId":"66006","inReplyTo":"20260827052318.GC176544@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] trace2: stop allowing die()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-08-31T13:27:49Z","receivedAt":"2026-08-31T13:27:52Z","isPatch":true,"body":"On 8/27/2026 1:23 AM, Jeff King wrote:\n> On Tue, Aug 25, 2026 at 06:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> This starts with a new banned-die.h header file at the root of the repo and\n>> including it from all trace2 API *.c files. It starts empty, but the later\n>> patches will add one method at a time:\n>>\n>> * xsnprintf() : This is the original patch, but made more complete by\n>> adding the method to banned-die.h.\n>> * xstrdup()\n>> * ALLOC_ARRAY()\n>> * xstrfmt()\n>> * ALLOC_GROW()\n>> * xcalloc()\n> \n> OK. This feels like the tip of the iceberg, though. All of strbuf would\n> have to be off-limits, too (both because it calls malloc directly, but\n> also because it will bail if snprintf() returns -1). I won't be\n> surprised if there are other indirect calls hiding in various places\n> (e.g., all of json-writer.c).\n\nYou're absolutely right. Not only in json-writer.c, but several direct\ncalls to the strbuf API. The only real way to fix that would be to\ncreate a \"safe strbuf\" library. This is potentially an interesting\ndirection that I might want to pursue and send an RFC after getting\nstarted. \n> I think if you really want to avoid allocations in trace2 it would\n> probably need to be a ground-up no-dependency rewrite.\n\nOr to update the dependencies to be \"safe\". Not an easy thing, either\nway.\n\nI don't have much knowledge of CodeQL, but the following vibe-coded\n.ql script is able to detect these transitive calls and demonstrate\nthe issue:\n\n----\n\nimport cpp\n\nclass Trace2Function extends Function {\n Trace2Function() {\n getFile().getRelativePath() = \"trace2.c\" or\n getFile().getRelativePath().matches(\"trace2/%.c\")\n }\n}\n\npredicate directlyCalls(Function caller, Function callee) {\n exists(FunctionCall call |\n call.getEnclosingFunction() = caller and\n call.getTarget() = callee\n )\n}\n\nfrom Trace2Function source, Function sink\nwhere\n sink.getName() = \"die\" and\n directlyCalls+(source, sink)\nselect source, \"This Trace2 function can transitively reach die().\"\n\n----\n\nAdding such a check now would obviously fail and not provide any\nability to demonstrate incremental progress like banned-die.h.\n\nI know that microsoft/git is running CodeQL analysis to look for\nsecurity issues [1] but doesn't appear to be running specific\nqueries like this one.\n\n[1] https://github.com/microsoft/git/commit/6b367b94752b7ae0fada0629a542e90ea0a1892c\n\nPerhaps this is something we could investigate in the future.\n\nThanks,\n-Stolee\n\n"},{"id":"551555","messageId":"apWB_D9oivo56vcw@pks.im","threadId":"66006","inReplyTo":"xmqqh5kikkgi.fsf@gitster.g","subject":"Re: [PATCH v2 1/7] banned-die: create header for banning of functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-31T13:30:36Z","receivedAt":"2026-08-31T13:30:50Z","isPatch":true,"body":"On Tue, Aug 25, 2026 at 01:34:53PM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n> > From: Derrick Stolee \n> >\n> > We have universally-banned functions listed in banned.h since\n> > c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\n> > the code should be more strict than others.\n> >\n> > One such example is the trace2 API which runs during atexit() and can\n> > prove to cause die()-handler recursion problems if it calls die().\n> >\n> > Create a new banned-die.h header file that will ban some Git methods\n> > that call die(). Include that in all trace2 API implementation files.\n> > This currently only bans die() itself, and that was already not used.\n> >\n> > It would be reasonable to name this file trace2/tr2_banned.h to be\n> > specific to the trace2 API, but it seems like such a restriction would\n> > be valuable to put in some other areas of the code, so adding it at the\n> > root of the tree seems like a good long-term approach.\n> \n> In other words, the functions banned by including this file are not\n> listed because they are banned from being used in trace2 API, but\n> because they may lead to die(). There may be some other traits that\n> we might want to avoid in certain subset of our code, and we may\n> have similar banned-frotz.h header to prevent direct or indirect use\n> of frotz. Which makes sense to me.\n> \n> Would the same approach work for the_hash_algo and the_repository, I\n> wonder?\n\nDon't we already do this? If `USE_THE_REPOSITORY_VARIABLE` is not\ndefined then we hide several function declarations where we know that\nthey depend on `the_repository`. It's not perfect as we still expose\nfunctions that do rely on it implicitly, but it's easy to remove more\nfunction declarations over time by just adding another ifdef.\n\nMaybe we should follow a similar approach with functions that die?\n\nPatrick\n"},{"id":"551577","messageId":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.git.1784131932489.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] trace2: stop allowing die()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:36Z","receivedAt":"2026-08-31T17:25:45Z","isPatch":true,"body":"NOTE: this v3 is rebased onto a recent 'master' due to conflicts in a test\nscript.\n\nAfter v1 was posted, based on a concrete example of tracing leading to a\nrecursive die() problem, more evidence has come up to imply that allocations\nare failing for some users more often. This is potentially an issue with the\nallocator chosen by Git for Windows, which is being discussed elsewhere.\n\nBut the conclusion is this: the trace2 API shouldn't call helpers that might\ncall die(). It's too low-level for that.\n\nIn this v2, I have a much more robust approach to removing die() from the\ntrace2 API.\n\nThis starts with a new banned-die.h header file at the root of the repo and\nincluding it from all trace2 API *.c files. It starts empty, but the later\npatches will add one method at a time:\n\n * xsnprintf() : This is the original patch, but made more complete by\n adding the method to banned-die.h.\n * xstrdup()\n * ALLOC_ARRAY()\n * xstrfmt()\n * ALLOC_GROW()\n * xcalloc()\n\nDuring each patch, the goal was to have the trace2 logic be \"as correct as\npossible\" when an allocation failure occurs. This may mean that we have\nincomplete messages or dropped trace messages.\n\nThe focus here is that the trace2 API should never cause a process-ending\nfailure, because those failures will trigger trace2 API calls while\nreporting the failure.\n\n\nUpdates in V3\n=============\n\n * Peff correctly points out that this is far from complete, as the strbuf\n library is not safe from die(). The banned-die.h provides incremental\n demonstration that these changes are showing progress and preventing\n regression in future changes, but not showing a complete picture. I will\n start an investigation into a \"safe\" or \"gentle\" variant of the strbuf\n API as a potential direction for these API layers.\n * The first patch had a lowercase banned() that should have been uppercase\n BANNED().\n * A 'return -1' was replaced with 'return 0' to avoid a misleading error\n message.\n * The \":\" string length was incorrect. This is fixed and tests\n are improved to cover this string manipulation. These test changes\n conflict with changes to use test_grep in 47f79f61983 (t: convert grep\n assertions to test_grep, 2026-07-06), so this v3 is rebased onto\n 'master'.\n * Patch 6 was previously failing at runtime. The appropriate fix is pulled\n out of patch 7 and into patch 6.\n\nThanks, -Stolee\n\nDerrick Stolee (7):\n banned-die: create header for banning of functions\n trace2: tolerate failed timestamp formatting\n trace2: remove use of xstrdup()\n trace2: remove use of ALLOC_ARRAY()\n trace2: remove use of xstrfmt()\n trace2: remove use of ALLOC_GROW()\n trace2: remove use of xcalloc()\n\n banned-die.h | 32 +++++++++++++++++\n t/t0212-trace2-event.sh | 12 ++++---\n trace2.c | 52 +++++++++++++++++++++++++---\n trace2/tr2_cfg.c | 2 ++\n trace2/tr2_cmd_name.c | 2 ++\n trace2/tr2_ctr.c | 12 ++++++-\n trace2/tr2_dst.c | 2 ++\n trace2/tr2_sid.c | 2 ++\n trace2/tr2_sysenv.c | 8 +++--\n trace2/tr2_tbuf.c | 51 +++++++++++++++++++--------\n trace2/tr2_tgt_event.c | 2 ++\n trace2/tr2_tgt_normal.c | 2 ++\n trace2/tr2_tgt_perf.c | 2 ++\n trace2/tr2_tls.c | 77 +++++++++++++++++++++++++++++++++++++++--\n trace2/tr2_tls.h | 7 ++++\n trace2/tr2_tmr.c | 16 +++++++--\n 16 files changed, 250 insertions(+), 31 deletions(-)\n create mode 100644 banned-die.h\n\n\nbase-commit: c73e85354c275c9d409b26445089bc16940fc527\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/2178\n\nRange-diff vs v2:\n\n 1: 84634717e2 ! 1: c483a4bf76 banned-die: create header for banning of functions\n @@ banned-die.h (new)\n + */\n +\n +#undef die\n -+#define die banned(die)\n ++#define die BANNED(die)\n +\n +#endif /* BANNED_DIE_H */\n \n @@ trace2.c\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n static int trace2_enabled;\n @@ trace2/tr2_cfg.c\n #include \"trace2/tr2_cfg.h\"\n #include \"trace2/tr2_sysenv.h\"\n #include \"wildmatch.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;\n @@ trace2/tr2_cmd_name.c\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n #include \"trace2/tr2_cmd_name.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_NAME \"GIT_TRACE2_PARENT_NAME\"\n @@ trace2/tr2_ctr.c\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_ctr.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n /*\n @@ trace2/tr2_dst.c\n #include \"trace2/tr2_dst.h\"\n #include \"trace2/tr2_sid.h\"\n #include \"trace2/tr2_sysenv.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n /*\n @@ trace2/tr2_sid.c\n #include \"strbuf.h\"\n #include \"trace2/tr2_tbuf.h\"\n #include \"trace2/tr2_sid.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_SID \"GIT_TRACE2_PARENT_SID\"\n @@ trace2/tr2_sysenv.c\n #include \"config.h\"\n #include \"dir.h\"\n #include \"tr2_sysenv.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n /*\n @@ trace2/tr2_tbuf.c\n @@\n #include \"git-compat-util.h\"\n #include \"tr2_tbuf.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n @@ trace2/tr2_tgt_event.c\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_event = {\n @@ trace2/tr2_tgt_normal.c\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_normal = {\n @@ trace2/tr2_tgt_perf.c\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_perf = {\n @@ trace2/tr2_tls.c\n #include \"thread-utils.h\"\n #include \"trace.h\"\n #include \"trace2/tr2_tls.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n /*\n @@ trace2/tr2_tmr.c\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n #include \"trace.h\"\n ++/* banned-die must be last. */\n +#include \"banned-die.h\"\n \n #define MY_MAX(a, b) ((a) > (b) ? (a) : (b))\n 2: bd45f46a34 ! 2: 754fffb74e trace2: tolerate failed timestamp formatting\n @@ Commit message\n ## banned-die.h ##\n @@\n #undef die\n - #define die banned(die)\n + #define die BANNED(die)\n \n +#undef xsnprintf\n +#define xsnprintf(...) BANNED(xsnprintf)\n 3: ec447a6a77 ! 3: 87d3f1b557 trace2: remove use of xstrdup()\n @@ Commit message\n trace2/tr2_sysenv.c.\n \n First, in tr2_sysenv_cb(), we need to handle a failed assignment of the\n - value with a negative return to halt the config parsing loop.\n + value with a zero-valued return to halt the config parsing loop. Note\n + that we don't want to use a negative return here or we would imply to\n + the config system that the config key or value was somehow invalid; such\n + an output would mask the real issue that the process failed to allocate\n + memory.\n \n Second, in tr2_sysenv_get(), the method will return NULL when strdup()\n returns NULL. This return is indistinguishable from the environment variable\n @@ Commit message\n failure at this level will likely lead to failure in another system, but at\n least the trace2 API will not cause the process to fail early.\n \n + Helped-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## banned-die.h ##\n @@ trace2/tr2_sysenv.c: static int tr2_sysenv_cb(const char *key, const char *value\n -\t\t\ttr2_sysenv_settings[k].value = xstrdup(value);\n +\t\t\ttr2_sysenv_settings[k].value = strdup(value);\n +\t\t\tif (!tr2_sysenv_settings[k].value)\n -+\t\t\t\treturn -1;\n ++\t\t\t\treturn 0;\n \t\t\treturn 0;\n \t\t}\n \t}\n 4: db6858d381 = 4: 5bf6ab91f3 trace2: remove use of ALLOC_ARRAY()\n 5: 7f0bb405ad ! 5: 3e419c5522 trace2: remove use of xstrfmt()\n @@ Commit message\n construct redacted data to avoid copying password information in traced\n URLs.\n \n + Update t0212 to more carefully test this behavior to explicitly include\n + the \":\" string in the appropriate context.\n +\n + Helped-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## banned-die.h ##\n @@ banned-die.h\n #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n \n \n + ## t/t0212-trace2-event.sh ##\n +@@ t/t0212-trace2-event.sh: test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '\n + \n + \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n + \t\ttest-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&\n +-\ttest_grep ! user:pwd trace.event\n ++\ttest_grep ! user:pwd trace.event &&\n ++\ttest_grep \"user:@example.com/\" trace.event\n + '\n + \n + test_expect_success 'unsafe URLs are redacted by default in child_start events' '\n +@@ t/t0212-trace2-event.sh: test_expect_success 'unsafe URLs are redacted by default in child_start events'\n + \n + \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n + \t\ttest-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&\n +-\ttest_grep ! user:pwd trace.event\n ++\ttest_grep ! user:pwd trace.event &&\n ++\ttest_grep \"user:@example.com/\" trace.event\n + '\n + \n + test_expect_success 'unsafe URLs are redacted by default in exec events' '\n +@@ t/t0212-trace2-event.sh: test_expect_success 'unsafe URLs are redacted by default in exec events' '\n + \n + \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n + \t\ttest-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&\n +-\ttest_grep ! user:pwd trace.event\n ++\ttest_grep ! user:pwd trace.event &&\n ++\ttest_grep \"user:@example.com/\" trace.event\n + '\n + \n + test_expect_success 'unsafe URLs are redacted by default in def_param events' '\n +@@ t/t0212-trace2-event.sh: test_expect_success 'unsafe URLs are redacted by default in def_param events' '\n + \n + \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n + \t\ttest-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&\n +-\ttest_grep ! user:pwd trace.event\n ++\ttest_grep ! user:pwd trace.event &&\n ++\ttest_grep \"user:@example.com/\" trace.event\n + '\n + \n + test_done\n +\n ## trace2.c ##\n @@ trace2.c: int trace2_is_enabled(void)\n static const char *redact_arg(const char *arg)\n @@ trace2.c: static const char *redact_arg(const char *arg)\n +\tsuffix_len = strlen(p + at);\n +\n +\tif (unsigned_add_overflows(prefix_len, suffix_len) ||\n -+\t unsigned_add_overflows(prefix_len + suffix_len, redact_len))\n ++\t unsigned_add_overflows(prefix_len + suffix_len, redact_len) ||\n ++\t unsigned_add_overflows(prefix_len + suffix_len + redact_len, 1))\n +\t\treturn NULL;\n +\n -+\tredacted_len = prefix_len + suffix_len + redact_len;\n ++\tredacted_len = prefix_len + suffix_len + redact_len + 1;\n +\n +\tredacted = malloc(redacted_len);\n +\tif (!redacted)\n +\t\treturn NULL;\n +\n +\tmemcpy(redacted, arg, prefix_len);\n -+\tmemcpy(redacted + prefix_len, redact, redact_len - 1);\n -+\tmemcpy(redacted + prefix_len + redact_len - 1, p + at,\n -+\t suffix_len + 1);\n ++\tmemcpy(redacted + prefix_len, redact, redact_len);\n ++\tmemcpy(redacted + prefix_len + redact_len, p + at, suffix_len + 1);\n +\treturn redacted;\n }\n \n 6: 120cf1967b ! 6: ccd284fbeb trace2: remove use of ALLOC_GROW()\n @@ Commit message\n deepening the stack, giving as much nesting behavior as possible without\n failing the entire process.\n \n + Helped-by: Elijah Newren \n Signed-off-by: Derrick Stolee \n \n ## banned-die.h ##\n @@ trace2/tr2_tls.c: void tr2tls_unset_self(void)\n +\t\treturn;\n +\t}\n +\n -+\tif (ctx->nr_open_regions < ctx->alloc)\n -+\t\treturn;\n ++\tif (ctx->nr_open_regions >= ctx->alloc) {\n ++\t\tif (ctx->alloc >\n ++\t\t SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n ++\t\t\tctx->nr_skipped_regions++;\n ++\t\t\treturn;\n ++\t\t}\n ++\t\tnew_alloc = ctx->alloc * 2;\n +\n -+\tif (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n -+\t\tctx->nr_skipped_regions++;\n -+\t\treturn;\n -+\t}\n -+\tnew_alloc = ctx->alloc * 2;\n ++\t\tnew_array = realloc(ctx->array_us_start,\n ++\t\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n ++\t\tif (!new_array) {\n ++\t\t\tctx->nr_skipped_regions++;\n ++\t\t\treturn;\n ++\t\t}\n +\n -+\tnew_array = realloc(ctx->array_us_start,\n -+\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n -+\tif (!new_array) {\n -+\t\tctx->nr_skipped_regions++;\n -+\t\treturn;\n ++\t\tctx->array_us_start = new_array;\n ++\t\tctx->alloc = new_alloc;\n +\t}\n -+\n -+\tctx->array_us_start = new_array;\n -+\tctx->alloc = new_alloc;\n \n -\tALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc);\n \tctx->array_us_start[ctx->nr_open_regions++] = us_now;\n 7: c8fc195a2a ! 7: fa10e8d246 trace2: remove use of xcalloc()\n @@ trace2/tr2_tls.c: void tr2tls_push_self(uint64_t us_now)\n \tuint64_t *new_array;\n \tsize_t new_alloc;\n \n --\tif (ctx->nr_skipped_regions) {\n --\t\tctx->nr_skipped_regions++;\n --\t\treturn;\n --\t}\n --\n --\tif (ctx->nr_open_regions < ctx->alloc)\n +\tif (tr2tls_is_fallback(ctx))\n - \t\treturn;\n - \n --\tif (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n -+\tif (ctx->nr_skipped_regions) {\n ++\t\treturn;\n ++\n + \tif (ctx->nr_skipped_regions) {\n \t\tctx->nr_skipped_regions++;\n \t\treturn;\n - \t}\n --\tnew_alloc = ctx->alloc * 2;\n - \n --\tnew_array = realloc(ctx->array_us_start,\n --\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n --\tif (!new_array) {\n --\t\tctx->nr_skipped_regions++;\n --\t\treturn;\n -+\tif (ctx->nr_open_regions >= ctx->alloc) {\n -+\t\tif (ctx->alloc >\n -+\t\t SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n -+\t\t\tctx->nr_skipped_regions++;\n -+\t\t\treturn;\n -+\t\t}\n -+\t\tnew_alloc = ctx->alloc * 2;\n -+\n -+\t\tnew_array = realloc(ctx->array_us_start,\n -+\t\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n -+\t\tif (!new_array) {\n -+\t\t\tctx->nr_skipped_regions++;\n -+\t\t\treturn;\n -+\t\t}\n -+\n -+\t\tctx->array_us_start = new_array;\n -+\t\tctx->alloc = new_alloc;\n - \t}\n - \n --\tctx->array_us_start = new_array;\n --\tctx->alloc = new_alloc;\n --\n - \tctx->array_us_start[ctx->nr_open_regions++] = us_now;\n - }\n - \n @@ trace2/tr2_tls.c: void tr2tls_pop_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n\n-- \ngitgitgadget\n"},{"id":"551578","messageId":"c483a4bf764c47ee2c7f715ff8143196cac73b9b.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] banned-die: create header for banning of functions","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:37Z","receivedAt":"2026-08-31T17:25:46Z","isPatch":true,"body":"From: Derrick Stolee \n\nWe have universally-banned functions listed in banned.h since\nc8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of\nthe code should be more strict than others.\n\nOne such example is the trace2 API which runs during atexit() and can\nprove to cause die()-handler recursion problems if it calls die().\n\nCreate a new banned-die.h header file that will ban some Git methods\nthat call die(). Include that in all trace2 API implementation files.\nThis currently only bans die() itself, and that was already not used.\n\nIt would be reasonable to name this file trace2/tr2_banned.h to be\nspecific to the trace2 API, but it seems like such a restriction would\nbe valuable to put in some other areas of the code, so adding it at the\nroot of the tree seems like a good long-term approach.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 14 ++++++++++++++\n trace2.c | 2 ++\n trace2/tr2_cfg.c | 2 ++\n trace2/tr2_cmd_name.c | 2 ++\n trace2/tr2_ctr.c | 2 ++\n trace2/tr2_dst.c | 2 ++\n trace2/tr2_sid.c | 2 ++\n trace2/tr2_sysenv.c | 2 ++\n trace2/tr2_tbuf.c | 2 ++\n trace2/tr2_tgt_event.c | 2 ++\n trace2/tr2_tgt_normal.c | 2 ++\n trace2/tr2_tgt_perf.c | 2 ++\n trace2/tr2_tls.c | 2 ++\n trace2/tr2_tmr.c | 2 ++\n 14 files changed, 40 insertions(+)\n create mode 100644 banned-die.h\n\ndiff --git a/banned-die.h b/banned-die.h\nnew file mode 100644\nindex 0000000000..1cde4035c1\n--- /dev/null\n+++ b/banned-die.h\n@@ -0,0 +1,14 @@\n+#ifndef BANNED_DIE_H\n+#define BANNED_DIE_H\n+\n+#include \"banned.h\"\n+\n+/*\n+ * This header lists functions that must not be used by low-level APIs\n+ * because they can cause Git to terminate.\n+ */\n+\n+#undef die\n+#define die BANNED(die)\n+\n+#endif /* BANNED_DIE_H */\ndiff --git a/trace2.c b/trace2.c\nindex c23c0a227b..8c974dee87 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -17,6 +17,8 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n static int trace2_enabled;\n static int trace2_redact = 1;\ndiff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c\nindex bbcfeda60a..757dfeae8d 100644\n--- a/trace2/tr2_cfg.c\n+++ b/trace2/tr2_cfg.c\n@@ -7,6 +7,8 @@\n #include \"trace2/tr2_cfg.h\"\n #include \"trace2/tr2_sysenv.h\"\n #include \"wildmatch.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;\n static int tr2_cfg_loaded;\ndiff --git a/trace2/tr2_cmd_name.c b/trace2/tr2_cmd_name.c\nindex b7b5a869b7..f378bef4cf 100644\n--- a/trace2/tr2_cmd_name.c\n+++ b/trace2/tr2_cmd_name.c\n@@ -1,6 +1,8 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n #include \"trace2/tr2_cmd_name.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_NAME \"GIT_TRACE2_PARENT_NAME\"\n \ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex ee17bfa86b..20618a65b2 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -2,6 +2,8 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_ctr.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n /*\n * A global counter block to aggregate values from the partial sums\ndiff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c\nindex 5be892cd5c..555ac7cb9e 100644\n--- a/trace2/tr2_dst.c\n+++ b/trace2/tr2_dst.c\n@@ -5,6 +5,8 @@\n #include \"trace2/tr2_dst.h\"\n #include \"trace2/tr2_sid.h\"\n #include \"trace2/tr2_sysenv.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n /*\n * How many attempts we will make at creating an automatically-named trace file.\ndiff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c\nindex 131b4f5a62..1d4f018f66 100644\n--- a/trace2/tr2_sid.c\n+++ b/trace2/tr2_sid.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"trace2/tr2_tbuf.h\"\n #include \"trace2/tr2_sid.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n #define TR2_ENVVAR_PARENT_SID \"GIT_TRACE2_PARENT_SID\"\n \ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex 4abc218514..7fa58eba91 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -4,6 +4,8 @@\n #include \"config.h\"\n #include \"dir.h\"\n #include \"tr2_sysenv.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n /*\n * Each entry represents a trace2 setting.\ndiff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c\nindex c3b3822ed7..d623e55a81 100644\n--- a/trace2/tr2_tbuf.c\n+++ b/trace2/tr2_tbuf.c\n@@ -1,5 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"tr2_tbuf.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n {\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 5a0381791f..36a746cc10 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -13,6 +13,8 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_event = {\n \t.sysenv_var = TR2_SYSENV_EVENT,\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex 924736ab36..82995e510f 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -11,6 +11,8 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_normal = {\n \t.sysenv_var = TR2_SYSENV_NORMAL,\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex 4eb9289f95..96a5bc7f10 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -14,6 +14,8 @@\n #include \"trace2/tr2_tgt.h\"\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n static struct tr2_dst tr2dst_perf = {\n \t.sysenv_var = TR2_SYSENV_PERF,\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 7b023c1bfc..49bd505d62 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -3,6 +3,8 @@\n #include \"thread-utils.h\"\n #include \"trace.h\"\n #include \"trace2/tr2_tls.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n /*\n * Initialize size of the thread stack for nested regions.\ndiff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c\nindex 038181ad9b..275091c693 100644\n--- a/trace2/tr2_tmr.c\n+++ b/trace2/tr2_tmr.c\n@@ -3,6 +3,8 @@\n #include \"trace2/tr2_tls.h\"\n #include \"trace2/tr2_tmr.h\"\n #include \"trace.h\"\n+/* banned-die must be last. */\n+#include \"banned-die.h\"\n \n #define MY_MAX(a, b) ((a) > (b) ? (a) : (b))\n #define MY_MIN(a, b) ((a) < (b) ? (a) : (b))\n-- \ngitgitgadget\n\n"},{"id":"551579","messageId":"754fffb74e05e8562321d94d17de51d4affab24f.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] trace2: tolerate failed timestamp formatting","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:38Z","receivedAt":"2026-08-31T17:25:47Z","isPatch":true,"body":"From: Derrick Stolee \n\nSome users reported issues of repeated messages:\n\n fatal: recursion detected in die handler\n\nThis wasn't happening every time, but we eventually captured a\nGIT_TRACE2_PERF log file with this issue and revealed an interesting\ninternal detail, failing with this message:\n\n unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\n\nThis specific format string tracks to tr2_tbuf_utc_datetime_extended()\nin trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in\nee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but\nwas later split in bad229aef23 (trace2: clarify UTC datetime formatting,\n2019-04-15).\n\nThis use of xsnprintf() is writing a very specific datetime format into a\n32-character buffer. The format requires that the input data will not\noverflow the format digits or the buffer will not hold the result. Since\nwe are using xsnprintf() here, those failures turn into die() events.\n\nThis method and its siblings, tr2_tbuf_local_time() and\ntr2_tbuf_utc_datetime(), are used in the tracing library. The extended\nform is used only for the 'event' format, which these users were using\nvia a config setting for use in client-side telemetry. The non-extended\nform is used to help generate the 'SID' that defines the process in the\ntraces.\n\nNot only are these inappropriate times for a failure, but the extended\nmethod is called specifially during the 'atexit' event, which was\ntriggering this problem in a loop as the 'atexit' event would be\nretriggered by the die().\n\nBased on other symptoms impacting users on the version reporting these\nfailures, it is most likely that this is actually a failure to allocate\nmemory, which is a specific symptom in Git for Windows. That fork uses a\ndifferent library for its implementation of vsprintf() which allocates\nan array when seven or more positional arguments exist in the formatting\nstring, such as this one.\n\nUltimately, the trace2 machinery is so low-level that it should not rely on\nany helper functions that perform error handling with die(), as that can\ntrigger issues that would then be traced, causing this kind of recursive\nloop.\n\nThese changes help remove any use of die() within this file:\n\n1. Both 'tv' and 'tm' structs are initialized with zero values, allowing\n an erroring gettimeofday() or gmtime_r() method to leave them\n zero-valued. A zero-valued date is better than a die() here.\n\n2. Replace the use of xsnprintf() with snprintf() to avoid the\n possibility of calling die() here. Instead, check the response to see\n if there was a failure. On failure, put a blank value into the buffer\n instead of possibly allowing a value that would not format correctly\n for a trace2 consumer. This value should be seen as obviously wrong\n and therefore signals a problem.\n\nAs the core issue in this code seems to require a system method\nreturning an error, no test accompanies this change.\n\nThis change removes all uses of xsnprintf() from the trace2/ directory.\nThere are two uses of xstrdup() that could be considered for removal,\nbut they only die() on out-of-memory errors instead of formatting\nissues. I chose to leave those in place for now.\n\nHelped-by: Taylor Blau \nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------\n 2 files changed, 37 insertions(+), 15 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 1cde4035c1..589e9cc2bd 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -11,4 +11,7 @@\n #undef die\n #define die BANNED(die)\n \n+#undef xsnprintf\n+#define xsnprintf(...) BANNED(xsnprintf)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c\nindex d623e55a81..9b9cdab025 100644\n--- a/trace2/tr2_tbuf.c\n+++ b/trace2/tr2_tbuf.c\n@@ -5,45 +5,64 @@\n \n void tr2_tbuf_local_time(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tlocaltime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\", tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf), \"%02d:%02d:%02d.%06ld\",\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"00:00:00.000000\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf),\n-\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\", tm.tm_year + 1900,\n-\t\t tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,\n-\t\t (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"1900-00-00T00:00:00.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n \n void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)\n {\n-\tstruct timeval tv;\n-\tstruct tm tm;\n+\tstruct timeval tv = { 0 };\n+\tstruct tm tm = { 0 };\n \ttime_t secs;\n+\tint len;\n \n \tgettimeofday(&tv, NULL);\n \tsecs = tv.tv_sec;\n \tgmtime_r(&secs, &tm);\n \n-\txsnprintf(tb->buf, sizeof(tb->buf), \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n-\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,\n-\t\t tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\tlen = snprintf(tb->buf, sizeof(tb->buf),\n+\t\t \"%4d%02d%02dT%02d%02d%02d.%06ldZ\",\n+\t\t tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,\n+\t\t tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);\n+\n+\tif (len < 0 || (size_t)len >= sizeof(tb->buf)) {\n+\t\tconst char *blank = \"19000000T000000.000000Z\";\n+\t\tstrlcpy(tb->buf, blank, sizeof(tb->buf));\n+\t}\n }\n-- \ngitgitgadget\n\n"},{"id":"551580","messageId":"87d3f1b557a1cddd08df1e7ba403602d2e8aba9b.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] trace2: remove use of xstrdup()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:39Z","receivedAt":"2026-08-31T17:25:49Z","isPatch":true,"body":"From: Derrick Stolee \n\nIn the previous change, we removed a use of xsprintf() that caused a\nrecursive die() loop when failing to allocate memory. The trace2 library is\ntoo low-level to be calling die(), especially because of these recursive\nloops that can occur during the die handler.\n\nFor full defense in depth, we remove the xstrdup() calls from\ntrace2/tr2_sysenv.c.\n\nFirst, in tr2_sysenv_cb(), we need to handle a failed assignment of the\nvalue with a zero-valued return to halt the config parsing loop. Note\nthat we don't want to use a negative return here or we would imply to\nthe config system that the config key or value was somehow invalid; such\nan output would mask the real issue that the process failed to allocate\nmemory.\n\nSecond, in tr2_sysenv_get(), the method will return NULL when strdup()\nreturns NULL. This return is indistinguishable from the environment variable\nhaving no value. That means that all callers know how to handle a NULL\nresponse, but no behavior change will occur between the case of no\nenvironment being set and detecting an environment variable exists but we\nfail to duplicate it. This seems an appropriate trade-off, as an allocation\nfailure at this level will likely lead to failure in another system, but at\nleast the trace2 API will not cause the process to fail early.\n\nHelped-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_sysenv.c | 6 ++++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 589e9cc2bd..bf16ec5ba9 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -14,4 +14,7 @@\n #undef xsnprintf\n #define xsnprintf(...) BANNED(xsnprintf)\n \n+#undef xstrdup\n+#define xstrdup(str) BANNED(xstrdup)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex 7fa58eba91..4a9983caf4 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -75,7 +75,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tfree(tr2_sysenv_settings[k].value);\n-\t\t\ttr2_sysenv_settings[k].value = xstrdup(value);\n+\t\t\ttr2_sysenv_settings[k].value = strdup(value);\n+\t\t\tif (!tr2_sysenv_settings[k].value)\n+\t\t\t\treturn 0;\n \t\t\treturn 0;\n \t\t}\n \t}\n@@ -111,7 +113,7 @@ const char *tr2_sysenv_get(enum tr2_sysenv_variable var)\n \t\tconst char *v = getenv(tr2_sysenv_settings[var].env_var_name);\n \t\tif (v && *v) {\n \t\t\tfree(tr2_sysenv_settings[var].value);\n-\t\t\ttr2_sysenv_settings[var].value = xstrdup(v);\n+\t\t\ttr2_sysenv_settings[var].value = strdup(v);\n \t\t}\n \t\ttr2_sysenv_settings[var].getenv_called = 1;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"551581","messageId":"5bf6ab91f37cd80dba5e50eee85f9f3258f34be3.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] trace2: remove use of ALLOC_ARRAY()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:40Z","receivedAt":"2026-08-31T17:25:51Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe banned-die.h header is used to prevent use of helper methods that\ncall die(). Remove use of the ALLOC_ARRAY() helper, which calls die() on\nallocation failures. Replace the use in trace2.c with a more direct\nallocation and soft failure when allocation fails. This prevents die()\nrecursion loops when memory allocation fails and trace2 logs are\nenabled.\n\nThe tricky part about this change is how to handle the results from\nredact_arg(), which is a 'const char *' result because it might be a\npointer directly to the externally-controlled argument. When it is\ndifferent from the argument, then it is indeed a newly-allocated string\nthat we need to free before returning. This requires using a (char *)\ncast to allow a change.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2.c | 16 ++++++++++++++--\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex bf16ec5ba9..0ad9a6c492 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,4 +17,7 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef ALLOC_ARRAY\n+#define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2.c b/trace2.c\nindex 8c974dee87..ea021c602e 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -305,7 +305,11 @@ static const char **redact_argv(const char **argv)\n \tfor (j = 0; argv[j]; j++)\n \t\t; /* keep counting */\n \n-\tALLOC_ARRAY(ret, j + 1);\n+\tret = calloc(j + 1, sizeof(*ret));\n+\tif (!ret) {\n+\t\tfree((char *)redacted);\n+\t\treturn NULL;\n+\t}\n \tret[j] = NULL;\n \n \tfor (j = 0; j < i; j++)\n@@ -346,6 +350,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)\n \tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n \n \tredacted = redact_argv(argv);\n+\tif (!redacted)\n+\t\treturn;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_start_fl)\n@@ -514,6 +520,7 @@ void trace2_child_start_fl(const char *file, int line,\n \tuint64_t us_now;\n \tuint64_t us_elapsed_absolute;\n \tconst char **orig_argv = cmd->args.v;\n+\tconst char **redacted;\n \n \tif (!trace2_enabled)\n \t\treturn;\n@@ -531,7 +538,10 @@ void trace2_child_start_fl(const char *file, int line,\n \t * temporarily replace the original argv (inside the `strvec`)\n \t * with a possibly redacted version.\n \t */\n-\tcmd->args.v = redact_argv(orig_argv);\n+\tredacted = redact_argv(orig_argv);\n+\tif (!redacted)\n+\t\treturn;\n+\tcmd->args.v = redacted;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_child_start_fl)\n@@ -623,6 +633,8 @@ int trace2_exec_fl(const char *file, int line, const char *exe,\n \texec_id = tr2tls_locked_increment(&tr2_next_exec_id);\n \n \tredacted = redact_argv(argv);\n+\tif (!redacted)\n+\t\treturn exec_id;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_exec_fl)\n-- \ngitgitgadget\n\n"},{"id":"551582","messageId":"3e419c55225f452f9472eac2cfb82aadb680a41e.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] trace2: remove use of xstrfmt()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:41Z","receivedAt":"2026-08-31T17:25:52Z","isPatch":true,"body":"From: Derrick Stolee \n\nWe continue removing the possibility of a die() in the trace2 API by\nbanning xstrfmt(), which calls die() during a failure to format. Instead\nof allowing a die(), perform a soft failure by failing to output the\ntrace2 data when such a failure occurs.\n\nThis requires carefully concatenating strings using memcpy() to\nconstruct redacted data to avoid copying password information in traced\nURLs.\n\nUpdate t0212 to more carefully test this behavior to explicitly include\nthe \":\" string in the appropriate context.\n\nHelped-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n t/t0212-trace2-event.sh | 12 ++++++++----\n trace2.c | 34 ++++++++++++++++++++++++++++++++--\n 3 files changed, 43 insertions(+), 6 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 0ad9a6c492..4d1800353d 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,6 +17,9 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef xstrfmt\n+#define xstrfmt(...) BANNED(xstrfmt)\n+\n #undef ALLOC_ARRAY\n #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n \ndiff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh\nindex f5358a1dd4..23df800395 100755\n--- a/t/t0212-trace2-event.sh\n+++ b/t/t0212-trace2-event.sh\n@@ -332,7 +332,8 @@ test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '\n \n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n \t\ttest-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&\n-\ttest_grep ! user:pwd trace.event\n+\ttest_grep ! user:pwd trace.event &&\n+\ttest_grep \"user:@example.com/\" trace.event\n '\n \n test_expect_success 'unsafe URLs are redacted by default in child_start events' '\n@@ -341,7 +342,8 @@ test_expect_success 'unsafe URLs are redacted by default in child_start events'\n \n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n \t\ttest-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&\n-\ttest_grep ! user:pwd trace.event\n+\ttest_grep ! user:pwd trace.event &&\n+\ttest_grep \"user:@example.com/\" trace.event\n '\n \n test_expect_success 'unsafe URLs are redacted by default in exec events' '\n@@ -350,7 +352,8 @@ test_expect_success 'unsafe URLs are redacted by default in exec events' '\n \n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n \t\ttest-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&\n-\ttest_grep ! user:pwd trace.event\n+\ttest_grep ! user:pwd trace.event &&\n+\ttest_grep \"user:@example.com/\" trace.event\n '\n \n test_expect_success 'unsafe URLs are redacted by default in def_param events' '\n@@ -359,7 +362,8 @@ test_expect_success 'unsafe URLs are redacted by default in def_param events' '\n \n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n \t\ttest-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&\n-\ttest_grep ! user:pwd trace.event\n+\ttest_grep ! user:pwd trace.event &&\n+\ttest_grep \"user:@example.com/\" trace.event\n '\n \n test_done\ndiff --git a/trace2.c b/trace2.c\nindex ea021c602e..4a597d8213 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -261,7 +261,10 @@ int trace2_is_enabled(void)\n static const char *redact_arg(const char *arg)\n {\n \tconst char *p, *colon;\n+\tconst char *redact = \":\";\n+\tchar *redacted;\n \tsize_t at;\n+\tsize_t prefix_len, suffix_len, redacted_len, redact_len;\n \n \tif (!trace2_redact ||\n \t (!skip_prefix(arg, \"https://\", &p) &&\n@@ -276,7 +279,25 @@ static const char *redact_arg(const char *arg)\n \tif (!colon)\n \t\treturn arg;\n \n-\treturn xstrfmt(\"%.*s:%s\", (int)(colon - arg), arg, p + at);\n+\tredact_len = strlen(redact);\n+\tprefix_len = colon - arg;\n+\tsuffix_len = strlen(p + at);\n+\n+\tif (unsigned_add_overflows(prefix_len, suffix_len) ||\n+\t unsigned_add_overflows(prefix_len + suffix_len, redact_len) ||\n+\t unsigned_add_overflows(prefix_len + suffix_len + redact_len, 1))\n+\t\treturn NULL;\n+\n+\tredacted_len = prefix_len + suffix_len + redact_len + 1;\n+\n+\tredacted = malloc(redacted_len);\n+\tif (!redacted)\n+\t\treturn NULL;\n+\n+\tmemcpy(redacted, arg, prefix_len);\n+\tmemcpy(redacted + prefix_len, redact, redact_len);\n+\tmemcpy(redacted + prefix_len + redact_len, p + at, suffix_len + 1);\n+\treturn redacted;\n }\n \n /*\n@@ -301,6 +322,8 @@ static const char **redact_argv(const char **argv)\n \n \tif (!argv[i])\n \t\treturn argv;\n+\tif (!redacted)\n+\t\treturn NULL;\n \n \tfor (j = 0; argv[j]; j++)\n \t\t; /* keep counting */\n@@ -317,7 +340,14 @@ static const char **redact_argv(const char **argv)\n \tret[i] = redacted;\n \tfor (++i; argv[i]; i++) {\n \t\tredacted = redact_arg(argv[i]);\n-\t\tret[i] = redacted ? redacted : argv[i];\n+\t\tif (!redacted) {\n+\t\t\tfor (j = 0; j < i; j++)\n+\t\t\t\tif (ret[j] != argv[j])\n+\t\t\t\t\tfree((void *)ret[j]);\n+\t\t\tfree(ret);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tret[i] = redacted;\n \t}\n \n \treturn ret;\n-- \ngitgitgadget\n\n"},{"id":"551583","messageId":"ccd284fbebcdc43812948bbd8b2d413dbf2b260d.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] trace2: remove use of ALLOC_GROW()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:42Z","receivedAt":"2026-08-31T17:25:53Z","isPatch":true,"body":"From: Derrick Stolee \n\nThe ALLOC_GROW() helper can call die() on a failed memory allocation.\nWe need to remove this from the trace2 API code to prevent a recursive\ndie() handler.\n\nThis helper is used to track the nested region stack. Use a new\nskipped_regions member to track how many times a region was entered\nwithout being added to the stack, and decrease that amount as we leave\neach region. This allows us to avoid a failure and instead stop\ndeepening the stack, giving as much nesting behavior as possible without\nfailing the entire process.\n\nHelped-by: Elijah Newren \nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_tls.c | 34 +++++++++++++++++++++++++++++++++-\n trace2/tr2_tls.h | 1 +\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex 4d1800353d..cff1072397 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -23,4 +23,7 @@\n #undef ALLOC_ARRAY\n #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)\n \n+#undef ALLOC_GROW\n+#define ALLOC_GROW(x, nr, alloc) BANNED(ALLOC_GROW)\n+\n #endif /* BANNED_DIE_H */\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 49bd505d62..5e4624d0b3 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -109,8 +109,33 @@ void tr2tls_unset_self(void)\n void tr2tls_push_self(uint64_t us_now)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n+\tuint64_t *new_array;\n+\tsize_t new_alloc;\n+\n+\tif (ctx->nr_skipped_regions) {\n+\t\tctx->nr_skipped_regions++;\n+\t\treturn;\n+\t}\n+\n+\tif (ctx->nr_open_regions >= ctx->alloc) {\n+\t\tif (ctx->alloc >\n+\t\t SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) {\n+\t\t\tctx->nr_skipped_regions++;\n+\t\t\treturn;\n+\t\t}\n+\t\tnew_alloc = ctx->alloc * 2;\n+\n+\t\tnew_array = realloc(ctx->array_us_start,\n+\t\t\t\t new_alloc * sizeof(*ctx->array_us_start));\n+\t\tif (!new_array) {\n+\t\t\tctx->nr_skipped_regions++;\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tctx->array_us_start = new_array;\n+\t\tctx->alloc = new_alloc;\n+\t}\n \n-\tALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc);\n \tctx->array_us_start[ctx->nr_open_regions++] = us_now;\n }\n \n@@ -118,6 +143,11 @@ void tr2tls_pop_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (ctx->nr_skipped_regions) {\n+\t\tctx->nr_skipped_regions--;\n+\t\treturn;\n+\t}\n+\n \tif (!ctx->nr_open_regions)\n \t\tBUG(\"no open regions in thread '%s'\", ctx->thread_name);\n \n@@ -138,6 +168,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)\n \tuint64_t us_start;\n \n \tctx = tr2tls_get_self();\n+\tif (ctx->nr_skipped_regions)\n+\t\treturn 0;\n \tif (!ctx->nr_open_regions)\n \t\treturn 0;\n \ndiff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h\nindex 3bdbf4d275..c365017923 100644\n--- a/trace2/tr2_tls.h\n+++ b/trace2/tr2_tls.h\n@@ -20,6 +20,7 @@ struct tr2tls_thread_ctx {\n \tuint64_t *array_us_start;\n \tsize_t alloc;\n \tsize_t nr_open_regions; /* plays role of \"nr\" in ALLOC_GROW */\n+\tsize_t nr_skipped_regions;\n \tint thread_id;\n \tstruct tr2_timer_block timer_block;\n \tstruct tr2_counter_block counter_block;\n-- \ngitgitgadget\n\n"},{"id":"551584","messageId":"fa10e8d246ccd7feb11422091b5ed4be3baf2ea5.1788197143.git.gitgitgadget@gmail.com","threadId":"66006","inReplyTo":"pull.2178.v3.git.1788197143.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] trace2: remove use of xcalloc()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-31T17:25:43Z","receivedAt":"2026-08-31T17:25:54Z","isPatch":true,"body":"From: Derrick Stolee \n\nRemove use of xcalloc() from the trace2 API due to its possible use of\ndie(), which could lead to recursive die() handlers. This is used in the\ntrace2 API to track an array of thread contexts when logging multi-\nthreaded operations.\n\nInstead of killing the process on a failure, we attempt to proceed as\nmuch as possible. We replace the dynamic thread context with a\nstatically-allocated context that uses the \"unknown\" thread name to\nidentify that we are in an error case.\n\nSigned-off-by: Derrick Stolee \n---\n banned-die.h | 3 +++\n trace2/tr2_ctr.c | 10 +++++++++-\n trace2/tr2_tls.c | 41 +++++++++++++++++++++++++++++++++++++++--\n trace2/tr2_tls.h | 6 ++++++\n trace2/tr2_tmr.c | 14 ++++++++++++--\n 5 files changed, 69 insertions(+), 5 deletions(-)\n\ndiff --git a/banned-die.h b/banned-die.h\nindex cff1072397..52a93c67c6 100644\n--- a/banned-die.h\n+++ b/banned-die.h\n@@ -17,6 +17,9 @@\n #undef xstrdup\n #define xstrdup(str) BANNED(xstrdup)\n \n+#undef xcalloc\n+#define xcalloc(nmemb, size) BANNED(xcalloc)\n+\n #undef xstrfmt\n #define xstrfmt(...) BANNED(xstrfmt)\n \ndiff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c\nindex 20618a65b2..9920979030 100644\n--- a/trace2/tr2_ctr.c\n+++ b/trace2/tr2_ctr.c\n@@ -55,7 +55,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER\n void tr2_counter_increment(enum trace2_counter_id cid, uint64_t value)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_counter *c = &ctx->counter_block.counter[cid];\n+\tstruct tr2_counter *c;\n+\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tc = &ctx->counter_block.counter[cid];\n \n \tc->value += value;\n \n@@ -69,6 +73,8 @@ void tr2_update_final_counters(void)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_counter_id cid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_counter)\n \t\treturn;\n \n@@ -90,6 +96,8 @@ void tr2_emit_per_thread_counters(tr2_tgt_evt_counter_t *fn_apply)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_counter_id cid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_per_thread_counter)\n \t\treturn;\n \ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 5e4624d0b3..ace2cd438b 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -14,6 +14,9 @@\n #define TR2_REGION_NESTING_INITIAL_SIZE (100)\n \n static struct tr2tls_thread_ctx *tr2tls_thread_main;\n+static struct tr2tls_thread_ctx tr2tls_thread_fallback = {\n+\t.thread_name = \"unknown\",\n+};\n static uint64_t tr2tls_us_start_process;\n \n static pthread_mutex_t tr2tls_mutex;\n@@ -38,16 +41,23 @@ void tr2tls_start_process_clock(void)\n struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n \t\t\t\t\t uint64_t us_thread_start)\n {\n-\tstruct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));\n+\tstruct tr2tls_thread_ctx *ctx = calloc(1, sizeof(*ctx));\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!ctx)\n+\t\tgoto fallback;\n+\n \t/*\n \t * Implicitly \"tr2tls_push_self()\" to capture the thread's start\n \t * time in array_us_start[0]. For the main thread this gives us the\n \t * application run time.\n \t */\n \tctx->alloc = TR2_REGION_NESTING_INITIAL_SIZE;\n-\tctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t));\n+\tctx->array_us_start = calloc(ctx->alloc, sizeof(uint64_t));\n+\tif (!ctx->array_us_start) {\n+\t\tfree(ctx);\n+\t\tgoto fallback;\n+\t}\n \tctx->array_us_start[ctx->nr_open_regions++] = us_thread_start;\n \n \tctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id);\n@@ -63,6 +73,10 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n \tpthread_setspecific(tr2tls_key, ctx);\n \n \treturn ctx;\n+\n+fallback:\n+\tpthread_setspecific(tr2tls_key, &tr2tls_thread_fallback);\n+\treturn &tr2tls_thread_fallback;\n }\n \n struct tr2tls_thread_ctx *tr2tls_get_self(void)\n@@ -85,6 +99,11 @@ struct tr2tls_thread_ctx *tr2tls_get_self(void)\n \treturn ctx;\n }\n \n+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx)\n+{\n+\treturn ctx == &tr2tls_thread_fallback;\n+}\n+\n int tr2tls_is_main_thread(void)\n {\n \tif (!HAVE_THREADS)\n@@ -101,6 +120,9 @@ void tr2tls_unset_self(void)\n \n \tpthread_setspecific(tr2tls_key, NULL);\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tfree((char *)ctx->thread_name);\n \tfree(ctx->array_us_start);\n \tfree(ctx);\n@@ -112,6 +134,9 @@ void tr2tls_push_self(uint64_t us_now)\n \tuint64_t *new_array;\n \tsize_t new_alloc;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tif (ctx->nr_skipped_regions) {\n \t\tctx->nr_skipped_regions++;\n \t\treturn;\n@@ -143,6 +168,9 @@ void tr2tls_pop_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tif (ctx->nr_skipped_regions) {\n \t\tctx->nr_skipped_regions--;\n \t\treturn;\n@@ -158,6 +186,9 @@ void tr2tls_pop_unwind_self(void)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \twhile (ctx->nr_open_regions > 1)\n \t\ttr2tls_pop_self();\n }\n@@ -168,6 +199,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us)\n \tuint64_t us_start;\n \n \tctx = tr2tls_get_self();\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn 0;\n \tif (ctx->nr_skipped_regions)\n \t\treturn 0;\n \tif (!ctx->nr_open_regions)\n@@ -189,6 +222,10 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us)\n static void tr2tls_key_destructor(void *payload)\n {\n \tstruct tr2tls_thread_ctx *ctx = payload;\n+\n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\n \tfree((char *)ctx->thread_name);\n \tfree(ctx->array_us_start);\n \tfree(ctx);\ndiff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h\nindex c365017923..4a0969c014 100644\n--- a/trace2/tr2_tls.h\n+++ b/trace2/tr2_tls.h\n@@ -54,6 +54,12 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name,\n */\n struct tr2tls_thread_ctx *tr2tls_get_self(void);\n \n+/*\n+ * Return true if the context is the non-allocating fallback used after an\n+ * allocation failure. Callers must not modify a fallback context.\n+ */\n+int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx);\n+\n /*\n * return true if the current thread is the main thread.\n */\ndiff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c\nindex 275091c693..4dfc7afb4e 100644\n--- a/trace2/tr2_tmr.c\n+++ b/trace2/tr2_tmr.c\n@@ -39,8 +39,11 @@ static struct tr2_timer_metadata tr2_timer_metadata[TRACE2_NUMBER_OF_TIMERS] = {\n void tr2_start_timer(enum trace2_timer_id tid)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_timer *t = &ctx->timer_block.timer[tid];\n+\tstruct tr2_timer *t;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tt = &ctx->timer_block.timer[tid];\n \tt->recursion_count++;\n \tif (t->recursion_count > 1)\n \t\treturn; /* ignore recursive starts */\n@@ -51,10 +54,13 @@ void tr2_start_timer(enum trace2_timer_id tid)\n void tr2_stop_timer(enum trace2_timer_id tid)\n {\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tstruct tr2_timer *t = &ctx->timer_block.timer[tid];\n+\tstruct tr2_timer *t;\n \tuint64_t ns_now;\n \tuint64_t ns_interval;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n+\tt = &ctx->timer_block.timer[tid];\n \tassert(t->recursion_count > 0);\n \n \tt->recursion_count--;\n@@ -92,6 +98,8 @@ void tr2_update_final_timers(void)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_timer_id tid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_timer)\n \t\treturn;\n \n@@ -138,6 +146,8 @@ void tr2_emit_per_thread_timers(tr2_tgt_evt_timer_t *fn_apply)\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n \tenum trace2_timer_id tid;\n \n+\tif (tr2tls_is_fallback(ctx))\n+\t\treturn;\n \tif (!ctx->used_any_per_thread_timer)\n \t\treturn;\n \n-- \ngitgitgadget\n"},{"id":"551621","messageId":"20260901050129.GB1075462@coredump.intra.peff.net","threadId":"66006","inReplyTo":"a41bdb3b-1fe7-4c1e-9d16-72390d93503b@gmail.com","subject":"Re: [PATCH v2 0/7] trace2: stop allowing die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-01T05:01:29Z","receivedAt":"2026-09-01T05:01:31Z","isPatch":true,"body":"On Mon, Aug 31, 2026 at 09:27:49AM -0400, Derrick Stolee wrote:\n\n> > OK. This feels like the tip of the iceberg, though. All of strbuf would\n> > have to be off-limits, too (both because it calls malloc directly, but\n> > also because it will bail if snprintf() returns -1). I won't be\n> > surprised if there are other indirect calls hiding in various places\n> > (e.g., all of json-writer.c).\n> \n> You're absolutely right. Not only in json-writer.c, but several direct\n> calls to the strbuf API. The only real way to fix that would be to\n> create a \"safe strbuf\" library. This is potentially an interesting\n> direction that I might want to pursue and send an RFC after getting\n> started.\n\nYes, though at some point the strbuf abstractions don't necessarily make\nsense, and you want to surface \"did we truncate\" or \"did this result\nfit\" to the caller.\n\nSo you probably end up with a whole new string interface (hopefully much\nmore stripped down than what strbuf needs).\n\n> > I think if you really want to avoid allocations in trace2 it would\n> > probably need to be a ground-up no-dependency rewrite.\n> \n> Or to update the dependencies to be \"safe\". Not an easy thing, either\n> way.\n\nYes. My thinking is that by the time you've pruned the dependencies,\nyou've essentially done that rewrite. So maybe it is all just a matter\nof perspective. One man's refactor is another's rewrite, or something. :)\n\n> I don't have much knowledge of CodeQL, but the following vibe-coded\n> .ql script is able to detect these transitive calls and demonstrate\n> the issue:\n\nYeah, I think the whack-a-mole can be solved with static analysis that\nactually understands the complete (possible) call tree. And then you\nwouldn't even really need your banned-die.h, because you'd have the real\nthing.\n\nThere's probably still a lot of work in rewriting the code to avoid\nthose dependencies, though. And I fear you may hit some part that really\nneeds to call into generic Git code in order to get an answer, which\nwill be hard to pull apart. But maybe not; in theory we are feeding data\ninto trace2, and it never really \"asks\" the rest of Git anything\nsubstantial.\n\n-Peff\n"},{"id":"551622","messageId":"20260901050311.GA1077240@coredump.intra.peff.net","threadId":"66006","inReplyTo":"20260901050129.GB1075462@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] trace2: stop allowing die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-01T05:03:11Z","receivedAt":"2026-09-01T05:03:13Z","isPatch":true,"body":"On Tue, Sep 01, 2026 at 01:01:30AM -0400, Jeff King wrote:\n\n> > I don't have much knowledge of CodeQL, but the following vibe-coded\n> > .ql script is able to detect these transitive calls and demonstrate\n> > the issue:\n> \n> Yeah, I think the whack-a-mole can be solved with static analysis that\n> actually understands the complete (possible) call tree. And then you\n> wouldn't even really need your banned-die.h, because you'd have the real\n> thing.\n\nJust to be clear, I am not opposed to banned-die.h in the meantime if it\nis helpful to your goals. The whack-a-mole is not something I would\nchoose to spend time on, but you are welcome to. ;)\n\n-Peff\n"},{"id":"551667","messageId":"c2c2d78a-52da-4814-9d05-ac757b164817@gmail.com","threadId":"66006","inReplyTo":"20260901050311.GA1077240@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] trace2: stop allowing die()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-09-01T13:42:46Z","receivedAt":"2026-09-01T13:42:50Z","isPatch":true,"body":"On 9/1/2026 1:03 AM, Jeff King wrote:\n> On Tue, Sep 01, 2026 at 01:01:30AM -0400, Jeff King wrote:\n> \n>>> I don't have much knowledge of CodeQL, but the following vibe-coded\n>>> .ql script is able to detect these transitive calls and demonstrate\n>>> the issue:\n>>\n>> Yeah, I think the whack-a-mole can be solved with static analysis that\n>> actually understands the complete (possible) call tree. And then you\n>> wouldn't even really need your banned-die.h, because you'd have the real\n>> thing.\n> \n> Just to be clear, I am not opposed to banned-die.h in the meantime if it\n> is helpful to your goals. The whack-a-mole is not something I would\n> choose to spend time on, but you are welcome to. ;)\nIt's helpful in the sense that it demonstrates progress during the\nrefactor, but it's less helpful as a long-term protection. Which you\npoint out quite well.\n\nI could easily send a v4 that removes patch 1 and all references to\nbanned-die.h with a focus on \"die() less in trace2\" to start this\nreduction, but with the knowledge that it isn't sufficient, yet.\n\nThanks,\n-Stolee\n\n"}]}