From 84634717e2eca479026d1cdf39a089a8f61d131e Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 12:06:16 -0400 Subject: [PATCH 1/7] banned-die: create header for banning of functions We have universally-banned functions listed in banned.h since c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of the code should be more strict than others. One such example is the trace2 API which runs during atexit() and can prove to cause die()-handler recursion problems if it calls die(). Create a new banned-die.h header file that will ban some Git methods that call die(). Include that in all trace2 API implementation files. This currently only bans die() itself, and that was already not used. It would be reasonable to name this file trace2/tr2_banned.h to be specific to the trace2 API, but it seems like such a restriction would be valuable to put in some other areas of the code, so adding it at the root of the tree seems like a good long-term approach. Signed-off-by: Derrick Stolee --- banned-die.h | 14 ++++++++++++++ trace2.c | 1 + trace2/tr2_cfg.c | 1 + trace2/tr2_cmd_name.c | 1 + trace2/tr2_ctr.c | 1 + trace2/tr2_dst.c | 1 + trace2/tr2_sid.c | 1 + trace2/tr2_sysenv.c | 1 + trace2/tr2_tbuf.c | 1 + trace2/tr2_tgt_event.c | 1 + trace2/tr2_tgt_normal.c | 1 + trace2/tr2_tgt_perf.c | 1 + trace2/tr2_tls.c | 1 + trace2/tr2_tmr.c | 1 + 14 files changed, 27 insertions(+) create mode 100644 banned-die.h diff --git a/banned-die.h b/banned-die.h new file mode 100644 index 00000000000000..5eff361e55efd7 --- /dev/null +++ b/banned-die.h @@ -0,0 +1,14 @@ +#ifndef BANNED_DIE_H +#define BANNED_DIE_H + +#include "banned.h" + +/* + * This header lists functions that must not be used by low-level APIs + * because they can cause Git to terminate. + */ + +#undef die +#define die banned(die) + +#endif /* BANNED_DIE_H */ diff --git a/trace2.c b/trace2.c index c23c0a227b7032..1d0ed2db2b51a2 100644 --- a/trace2.c +++ b/trace2.c @@ -17,6 +17,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" +#include "banned-die.h" static int trace2_enabled; static int trace2_redact = 1; diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c index bbcfeda60af4de..06912a3cebc041 100644 --- a/trace2/tr2_cfg.c +++ b/trace2/tr2_cfg.c @@ -7,6 +7,7 @@ #include "trace2/tr2_cfg.h" #include "trace2/tr2_sysenv.h" #include "wildmatch.h" +#include "banned-die.h" static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP; static int tr2_cfg_loaded; diff --git a/trace2/tr2_cmd_name.c b/trace2/tr2_cmd_name.c index b7b5a869b74bcd..88f24e8781f879 100644 --- a/trace2/tr2_cmd_name.c +++ b/trace2/tr2_cmd_name.c @@ -1,6 +1,7 @@ #include "git-compat-util.h" #include "strbuf.h" #include "trace2/tr2_cmd_name.h" +#include "banned-die.h" #define TR2_ENVVAR_PARENT_NAME "GIT_TRACE2_PARENT_NAME" diff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c index ee17bfa86b401b..3067df4d18bc13 100644 --- a/trace2/tr2_ctr.c +++ b/trace2/tr2_ctr.c @@ -2,6 +2,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_ctr.h" +#include "banned-die.h" /* * A global counter block to aggregate values from the partial sums diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c index 5be892cd5cdefa..686a3e42fcd835 100644 --- a/trace2/tr2_dst.c +++ b/trace2/tr2_dst.c @@ -5,6 +5,7 @@ #include "trace2/tr2_dst.h" #include "trace2/tr2_sid.h" #include "trace2/tr2_sysenv.h" +#include "banned-die.h" /* * How many attempts we will make at creating an automatically-named trace file. diff --git a/trace2/tr2_sid.c b/trace2/tr2_sid.c index 1c1d27b0eee935..358f61b301695b 100644 --- a/trace2/tr2_sid.c +++ b/trace2/tr2_sid.c @@ -3,6 +3,7 @@ #include "strbuf.h" #include "trace2/tr2_tbuf.h" #include "trace2/tr2_sid.h" +#include "banned-die.h" #define TR2_ENVVAR_PARENT_SID "GIT_TRACE2_PARENT_SID" diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c index 4abc218514fdbc..deb3fabff429d4 100644 --- a/trace2/tr2_sysenv.c +++ b/trace2/tr2_sysenv.c @@ -4,6 +4,7 @@ #include "config.h" #include "dir.h" #include "tr2_sysenv.h" +#include "banned-die.h" /* * Each entry represents a trace2 setting. diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c index c3b3822ed7e4af..86725426f65b87 100644 --- a/trace2/tr2_tbuf.c +++ b/trace2/tr2_tbuf.c @@ -1,5 +1,6 @@ #include "git-compat-util.h" #include "tr2_tbuf.h" +#include "banned-die.h" void tr2_tbuf_local_time(struct tr2_tbuf *tb) { diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 5a0381791f7eb4..a055e19bace9c8 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -13,6 +13,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" +#include "banned-die.h" static struct tr2_dst tr2dst_event = { .sysenv_var = TR2_SYSENV_EVENT, diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index 924736ab36093b..97d4c5d2023089 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -11,6 +11,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" +#include "banned-die.h" static struct tr2_dst tr2dst_normal = { .sysenv_var = TR2_SYSENV_NORMAL, diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index 4eb9289f950505..1f49d9f9221bba 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -14,6 +14,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" +#include "banned-die.h" static struct tr2_dst tr2dst_perf = { .sysenv_var = TR2_SYSENV_PERF, diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c index 7b023c1bfc65fa..ae2d39d2f521e4 100644 --- a/trace2/tr2_tls.c +++ b/trace2/tr2_tls.c @@ -3,6 +3,7 @@ #include "thread-utils.h" #include "trace.h" #include "trace2/tr2_tls.h" +#include "banned-die.h" /* * Initialize size of the thread stack for nested regions. diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c index 038181ad9be05b..a329c466b92a31 100644 --- a/trace2/tr2_tmr.c +++ b/trace2/tr2_tmr.c @@ -3,6 +3,7 @@ #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" #include "trace.h" +#include "banned-die.h" #define MY_MAX(a, b) ((a) > (b) ? (a) : (b)) #define MY_MIN(a, b) ((a) < (b) ? (a) : (b)) From bd45f46a34aeb539d45d71b76c12c319abbfbcb5 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 13 Jul 2026 13:35:33 -0400 Subject: [PATCH 2/7] trace2: tolerate failed timestamp formatting Some users reported issues of repeated messages: fatal: recursion detected in die handler This wasn't happening every time, but we eventually captured a GIT_TRACE2_PERF log file with this issue and revealed an interesting internal detail, failing with this message: unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ This specific format string tracks to tr2_tbuf_utc_datetime_extended() in trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but was later split in bad229aef23 (trace2: clarify UTC datetime formatting, 2019-04-15). This use of xsnprintf() is writing a very specific datetime format into a 32-character buffer. The format requires that the input data will not overflow the format digits or the buffer will not hold the result. Since we are using xsnprintf() here, those failures turn into die() events. This method and its siblings, tr2_tbuf_local_time() and tr2_tbuf_utc_datetime(), are used in the tracing library. The extended form is used only for the 'event' format, which these users were using via a config setting for use in client-side telemetry. The non-extended form is used to help generate the 'SID' that defines the process in the traces. Not only are these inappropriate times for a failure, but the extended method is called specifially during the 'atexit' event, which was triggering this problem in a loop as the 'atexit' event would be retriggered by the die(). Based on other symptoms impacting users on the version reporting these failures, it is most likely that this is actually a failure to allocate memory, which is a specific symptom in Git for Windows. That fork uses a different library for its implementation of vsprintf() which allocates an array when seven or more positional arguments exist in the formatting string, such as this one. Ultimately, the trace2 machinery is so low-level that it should not rely on any helper functions that perform error handling with die(), as that can trigger issues that would then be traced, causing this kind of recursive loop. These changes help remove any use of die() within this file: 1. Both 'tv' and 'tm' structs are initialized with zero values, allowing an erroring gettimeofday() or gmtime_r() method to leave them zero-valued. A zero-valued date is better than a die() here. 2. Replace the use of xsnprintf() with snprintf() to avoid the possibility of calling die() here. Instead, check the response to see if there was a failure. On failure, put a blank value into the buffer instead of possibly allowing a value that would not format correctly for a trace2 consumer. This value should be seen as obviously wrong and therefore signals a problem. As the core issue in this code seems to require a system method returning an error, no test accompanies this change. This change removes all uses of xsnprintf() from the trace2/ directory. There are two uses of xstrdup() that could be considered for removal, but they only die() on out-of-memory errors instead of formatting issues. I chose to leave those in place for now. Helped-by: Taylor Blau Signed-off-by: Derrick Stolee --- banned-die.h | 3 +++ trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/banned-die.h b/banned-die.h index 5eff361e55efd7..0e0a794e5d3cd4 100644 --- a/banned-die.h +++ b/banned-die.h @@ -11,4 +11,7 @@ #undef die #define die banned(die) +#undef xsnprintf +#define xsnprintf(...) BANNED(xsnprintf) + #endif /* BANNED_DIE_H */ diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c index 86725426f65b87..fff345cb99f9e7 100644 --- a/trace2/tr2_tbuf.c +++ b/trace2/tr2_tbuf.c @@ -4,45 +4,64 @@ void tr2_tbuf_local_time(struct tr2_tbuf *tb) { - struct timeval tv; - struct tm tm; + struct timeval tv = { 0 }; + struct tm tm = { 0 }; time_t secs; + int len; gettimeofday(&tv, NULL); secs = tv.tv_sec; localtime_r(&secs, &tm); - xsnprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld", tm.tm_hour, - tm.tm_min, tm.tm_sec, (long)tv.tv_usec); + len = snprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld", + tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec); + + if (len < 0 || (size_t)len >= sizeof(tb->buf)) { + const char *blank = "00:00:00.000000"; + strlcpy(tb->buf, blank, sizeof(tb->buf)); + } } void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb) { - struct timeval tv; - struct tm tm; + struct timeval tv = { 0 }; + struct tm tm = { 0 }; time_t secs; + int len; gettimeofday(&tv, NULL); secs = tv.tv_sec; gmtime_r(&secs, &tm); - xsnprintf(tb->buf, sizeof(tb->buf), - "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ", tm.tm_year + 1900, - tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec, - (long)tv.tv_usec); + len = snprintf(tb->buf, sizeof(tb->buf), + "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ", + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec); + + if (len < 0 || (size_t)len >= sizeof(tb->buf)) { + const char *blank = "1900-00-00T00:00:00.000000Z"; + strlcpy(tb->buf, blank, sizeof(tb->buf)); + } } void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb) { - struct timeval tv; - struct tm tm; + struct timeval tv = { 0 }; + struct tm tm = { 0 }; time_t secs; + int len; gettimeofday(&tv, NULL); secs = tv.tv_sec; gmtime_r(&secs, &tm); - xsnprintf(tb->buf, sizeof(tb->buf), "%4d%02d%02dT%02d%02d%02d.%06ldZ", - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, - tm.tm_min, tm.tm_sec, (long)tv.tv_usec); + len = snprintf(tb->buf, sizeof(tb->buf), + "%4d%02d%02dT%02d%02d%02d.%06ldZ", + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, + tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec); + + if (len < 0 || (size_t)len >= sizeof(tb->buf)) { + const char *blank = "19000000T000000.000000Z"; + strlcpy(tb->buf, blank, sizeof(tb->buf)); + } } From ec447a6a778a5c49344346df54b434a96c792082 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 11:25:05 -0400 Subject: [PATCH 3/7] trace2: remove use of xstrdup() In the previous change, we removed a use of xsprintf() that caused a recursive die() loop when failing to allocate memory. The trace2 library is too low-level to be calling die(), especially because of these recursive loops that can occur during the die handler. For full defense in depth, we remove the xstrdup() calls from trace2/tr2_sysenv.c. First, in tr2_sysenv_cb(), we need to handle a failed assignment of the value with a negative return to halt the config parsing loop. Second, in tr2_sysenv_get(), the method will return NULL when strdup() returns NULL. This return is indistinguishable from the environment variable having no value. That means that all callers know how to handle a NULL response, but no behavior change will occur between the case of no environment being set and detecting an environment variable exists but we fail to duplicate it. This seems an appropriate trade-off, as an allocation failure at this level will likely lead to failure in another system, but at least the trace2 API will not cause the process to fail early. Signed-off-by: Derrick Stolee --- banned-die.h | 3 +++ trace2/tr2_sysenv.c | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/banned-die.h b/banned-die.h index 0e0a794e5d3cd4..2e16c4899cd49a 100644 --- a/banned-die.h +++ b/banned-die.h @@ -14,4 +14,7 @@ #undef xsnprintf #define xsnprintf(...) BANNED(xsnprintf) +#undef xstrdup +#define xstrdup(str) BANNED(xstrdup) + #endif /* BANNED_DIE_H */ diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c index deb3fabff429d4..4ee273a4aedd11 100644 --- a/trace2/tr2_sysenv.c +++ b/trace2/tr2_sysenv.c @@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value, if (!value) return config_error_nonbool(key); free(tr2_sysenv_settings[k].value); - tr2_sysenv_settings[k].value = xstrdup(value); + tr2_sysenv_settings[k].value = strdup(value); + if (!tr2_sysenv_settings[k].value) + return -1; return 0; } } @@ -110,7 +112,7 @@ const char *tr2_sysenv_get(enum tr2_sysenv_variable var) const char *v = getenv(tr2_sysenv_settings[var].env_var_name); if (v && *v) { free(tr2_sysenv_settings[var].value); - tr2_sysenv_settings[var].value = xstrdup(v); + tr2_sysenv_settings[var].value = strdup(v); } tr2_sysenv_settings[var].getenv_called = 1; } From db6858d3811c8cfdd136a0069f0ace33b95888ae Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 12:08:52 -0400 Subject: [PATCH 4/7] trace2: remove use of ALLOC_ARRAY() The banned-die.h header is used to prevent use of helper methods that call die(). Remove use of the ALLOC_ARRAY() helper, which calls die() on allocation failures. Replace the use in trace2.c with a more direct allocation and soft failure when allocation fails. This prevents die() recursion loops when memory allocation fails and trace2 logs are enabled. The tricky part about this change is how to handle the results from redact_arg(), which is a 'const char *' result because it might be a pointer directly to the externally-controlled argument. When it is different from the argument, then it is indeed a newly-allocated string that we need to free before returning. This requires using a (char *) cast to allow a change. Signed-off-by: Derrick Stolee --- banned-die.h | 3 +++ trace2.c | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/banned-die.h b/banned-die.h index 2e16c4899cd49a..cb2eed75cda889 100644 --- a/banned-die.h +++ b/banned-die.h @@ -17,4 +17,7 @@ #undef xstrdup #define xstrdup(str) BANNED(xstrdup) +#undef ALLOC_ARRAY +#define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY) + #endif /* BANNED_DIE_H */ diff --git a/trace2.c b/trace2.c index 1d0ed2db2b51a2..704427643514f8 100644 --- a/trace2.c +++ b/trace2.c @@ -304,7 +304,11 @@ static const char **redact_argv(const char **argv) for (j = 0; argv[j]; j++) ; /* keep counting */ - ALLOC_ARRAY(ret, j + 1); + ret = calloc(j + 1, sizeof(*ret)); + if (!ret) { + free((char *)redacted); + return NULL; + } ret[j] = NULL; for (j = 0; j < i; j++) @@ -345,6 +349,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv) us_elapsed_absolute = tr2tls_absolute_elapsed(us_now); redacted = redact_argv(argv); + if (!redacted) + return; for_each_wanted_builtin (j, tgt_j) if (tgt_j->pfn_start_fl) @@ -513,6 +519,7 @@ void trace2_child_start_fl(const char *file, int line, uint64_t us_now; uint64_t us_elapsed_absolute; const char **orig_argv = cmd->args.v; + const char **redacted; if (!trace2_enabled) return; @@ -530,7 +537,10 @@ void trace2_child_start_fl(const char *file, int line, * temporarily replace the original argv (inside the `strvec`) * with a possibly redacted version. */ - cmd->args.v = redact_argv(orig_argv); + redacted = redact_argv(orig_argv); + if (!redacted) + return; + cmd->args.v = redacted; for_each_wanted_builtin (j, tgt_j) if (tgt_j->pfn_child_start_fl) @@ -622,6 +632,8 @@ int trace2_exec_fl(const char *file, int line, const char *exe, exec_id = tr2tls_locked_increment(&tr2_next_exec_id); redacted = redact_argv(argv); + if (!redacted) + return exec_id; for_each_wanted_builtin (j, tgt_j) if (tgt_j->pfn_exec_fl) From 7f0bb405ad380fd35ae6381961ac667fd7e5dfd9 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 12:09:17 -0400 Subject: [PATCH 5/7] trace2: remove use of xstrfmt() We continue removing the possibility of a die() in the trace2 API by banning xstrfmt(), which calls die() during a failure to format. Instead of allowing a die(), perform a soft failure by failing to output the trace2 data when such a failure occurs. This requires carefully concatenating strings using memcpy() to construct redacted data to avoid copying password information in traced URLs. Signed-off-by: Derrick Stolee --- banned-die.h | 3 +++ trace2.c | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/banned-die.h b/banned-die.h index cb2eed75cda889..14aecfdc7a8d63 100644 --- a/banned-die.h +++ b/banned-die.h @@ -17,6 +17,9 @@ #undef xstrdup #define xstrdup(str) BANNED(xstrdup) +#undef xstrfmt +#define xstrfmt(...) BANNED(xstrfmt) + #undef ALLOC_ARRAY #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY) diff --git a/trace2.c b/trace2.c index 704427643514f8..c37f783fa032a2 100644 --- a/trace2.c +++ b/trace2.c @@ -260,7 +260,10 @@ int trace2_is_enabled(void) static const char *redact_arg(const char *arg) { const char *p, *colon; + const char *redact = ":"; + char *redacted; size_t at; + size_t prefix_len, suffix_len, redacted_len, redact_len; if (!trace2_redact || (!skip_prefix(arg, "https://", &p) && @@ -275,7 +278,25 @@ static const char *redact_arg(const char *arg) if (!colon) return arg; - return xstrfmt("%.*s:%s", (int)(colon - arg), arg, p + at); + redact_len = strlen(redact); + prefix_len = colon - arg; + suffix_len = strlen(p + at); + + if (unsigned_add_overflows(prefix_len, suffix_len) || + unsigned_add_overflows(prefix_len + suffix_len, redact_len)) + return NULL; + + redacted_len = prefix_len + suffix_len + redact_len; + + redacted = malloc(redacted_len); + if (!redacted) + return NULL; + + memcpy(redacted, arg, prefix_len); + memcpy(redacted + prefix_len, redact, redact_len - 1); + memcpy(redacted + prefix_len + redact_len - 1, p + at, + suffix_len + 1); + return redacted; } /* @@ -300,6 +321,8 @@ static const char **redact_argv(const char **argv) if (!argv[i]) return argv; + if (!redacted) + return NULL; for (j = 0; argv[j]; j++) ; /* keep counting */ @@ -316,7 +339,14 @@ static const char **redact_argv(const char **argv) ret[i] = redacted; for (++i; argv[i]; i++) { redacted = redact_arg(argv[i]); - ret[i] = redacted ? redacted : argv[i]; + if (!redacted) { + for (j = 0; j < i; j++) + if (ret[j] != argv[j]) + free((void *)ret[j]); + free(ret); + return NULL; + } + ret[i] = redacted; } return ret; From 120cf1967bde4e719a781c391b285c718553ad58 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 12:29:24 -0400 Subject: [PATCH 6/7] trace2: remove use of ALLOC_GROW() The ALLOC_GROW() helper can call die() on a failed memory allocation. We need to remove this from the trace2 API code to prevent a recursive die() handler. This helper is used to track the nested region stack. Use a new skipped_regions member to track how many times a region was entered without being added to the stack, and decrease that amount as we leave each region. This allows us to avoid a failure and instead stop deepening the stack, giving as much nesting behavior as possible without failing the entire process. Signed-off-by: Derrick Stolee --- banned-die.h | 3 +++ trace2/tr2_tls.c | 34 +++++++++++++++++++++++++++++++++- trace2/tr2_tls.h | 1 + 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/banned-die.h b/banned-die.h index 14aecfdc7a8d63..423e7b607db5c7 100644 --- a/banned-die.h +++ b/banned-die.h @@ -23,4 +23,7 @@ #undef ALLOC_ARRAY #define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY) +#undef ALLOC_GROW +#define ALLOC_GROW(x, nr, alloc) BANNED(ALLOC_GROW) + #endif /* BANNED_DIE_H */ diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c index ae2d39d2f521e4..8596292a947e55 100644 --- a/trace2/tr2_tls.c +++ b/trace2/tr2_tls.c @@ -108,8 +108,33 @@ void tr2tls_unset_self(void) void tr2tls_push_self(uint64_t us_now) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); + uint64_t *new_array; + size_t new_alloc; + + if (ctx->nr_skipped_regions) { + ctx->nr_skipped_regions++; + return; + } + + if (ctx->nr_open_regions < ctx->alloc) + return; + + if (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) { + ctx->nr_skipped_regions++; + return; + } + new_alloc = ctx->alloc * 2; + + new_array = realloc(ctx->array_us_start, + new_alloc * sizeof(*ctx->array_us_start)); + if (!new_array) { + ctx->nr_skipped_regions++; + return; + } + + ctx->array_us_start = new_array; + ctx->alloc = new_alloc; - ALLOC_GROW(ctx->array_us_start, ctx->nr_open_regions + 1, ctx->alloc); ctx->array_us_start[ctx->nr_open_regions++] = us_now; } @@ -117,6 +142,11 @@ void tr2tls_pop_self(void) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); + if (ctx->nr_skipped_regions) { + ctx->nr_skipped_regions--; + return; + } + if (!ctx->nr_open_regions) BUG("no open regions in thread '%s'", ctx->thread_name); @@ -137,6 +167,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us) uint64_t us_start; ctx = tr2tls_get_self(); + if (ctx->nr_skipped_regions) + return 0; if (!ctx->nr_open_regions) return 0; diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h index 3bdbf4d2754b40..c365017923601f 100644 --- a/trace2/tr2_tls.h +++ b/trace2/tr2_tls.h @@ -20,6 +20,7 @@ struct tr2tls_thread_ctx { uint64_t *array_us_start; size_t alloc; size_t nr_open_regions; /* plays role of "nr" in ALLOC_GROW */ + size_t nr_skipped_regions; int thread_id; struct tr2_timer_block timer_block; struct tr2_counter_block counter_block; From c8fc195a2ace4c2058ffa87e40a5745d349ab2dc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sun, 2 Aug 2026 12:30:18 -0400 Subject: [PATCH 7/7] trace2: remove use of xcalloc() Remove use of xcalloc() from the trace2 API due to its possible use of die(), which could lead to recursive die() handlers. This is used in the trace2 API to track an array of thread contexts when logging multi- threaded operations. Instead of killing the process on a failure, we attempt to proceed as much as possible. We replace the dynamic thread context with a statically-allocated context that uses the "unknown" thread name to identify that we are in an error case. Signed-off-by: Derrick Stolee --- banned-die.h | 3 ++ trace2/tr2_ctr.c | 10 ++++++- trace2/tr2_tls.c | 73 ++++++++++++++++++++++++++++++++++++------------ trace2/tr2_tls.h | 6 ++++ trace2/tr2_tmr.c | 14 ++++++++-- 5 files changed, 85 insertions(+), 21 deletions(-) diff --git a/banned-die.h b/banned-die.h index 423e7b607db5c7..3dc521f6b01e5c 100644 --- a/banned-die.h +++ b/banned-die.h @@ -17,6 +17,9 @@ #undef xstrdup #define xstrdup(str) BANNED(xstrdup) +#undef xcalloc +#define xcalloc(nmemb, size) BANNED(xcalloc) + #undef xstrfmt #define xstrfmt(...) BANNED(xstrfmt) diff --git a/trace2/tr2_ctr.c b/trace2/tr2_ctr.c index 3067df4d18bc13..5283946e08aef8 100644 --- a/trace2/tr2_ctr.c +++ b/trace2/tr2_ctr.c @@ -54,7 +54,11 @@ static struct tr2_counter_metadata tr2_counter_metadata[TRACE2_NUMBER_OF_COUNTER void tr2_counter_increment(enum trace2_counter_id cid, uint64_t value) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); - struct tr2_counter *c = &ctx->counter_block.counter[cid]; + struct tr2_counter *c; + + if (tr2tls_is_fallback(ctx)) + return; + c = &ctx->counter_block.counter[cid]; c->value += value; @@ -68,6 +72,8 @@ void tr2_update_final_counters(void) struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); enum trace2_counter_id cid; + if (tr2tls_is_fallback(ctx)) + return; if (!ctx->used_any_counter) return; @@ -89,6 +95,8 @@ void tr2_emit_per_thread_counters(tr2_tgt_evt_counter_t *fn_apply) struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); enum trace2_counter_id cid; + if (tr2tls_is_fallback(ctx)) + return; if (!ctx->used_any_per_thread_counter) return; diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c index 8596292a947e55..2c6aaed5049e73 100644 --- a/trace2/tr2_tls.c +++ b/trace2/tr2_tls.c @@ -13,6 +13,9 @@ #define TR2_REGION_NESTING_INITIAL_SIZE (100) static struct tr2tls_thread_ctx *tr2tls_thread_main; +static struct tr2tls_thread_ctx tr2tls_thread_fallback = { + .thread_name = "unknown", +}; static uint64_t tr2tls_us_start_process; static pthread_mutex_t tr2tls_mutex; @@ -37,16 +40,23 @@ void tr2tls_start_process_clock(void) struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name, uint64_t us_thread_start) { - struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx)); + struct tr2tls_thread_ctx *ctx = calloc(1, sizeof(*ctx)); struct strbuf buf = STRBUF_INIT; + if (!ctx) + goto fallback; + /* * Implicitly "tr2tls_push_self()" to capture the thread's start * time in array_us_start[0]. For the main thread this gives us the * application run time. */ ctx->alloc = TR2_REGION_NESTING_INITIAL_SIZE; - ctx->array_us_start = (uint64_t *)xcalloc(ctx->alloc, sizeof(uint64_t)); + ctx->array_us_start = calloc(ctx->alloc, sizeof(uint64_t)); + if (!ctx->array_us_start) { + free(ctx); + goto fallback; + } ctx->array_us_start[ctx->nr_open_regions++] = us_thread_start; ctx->thread_id = tr2tls_locked_increment(&tr2_next_thread_id); @@ -62,6 +72,10 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name, pthread_setspecific(tr2tls_key, ctx); return ctx; + +fallback: + pthread_setspecific(tr2tls_key, &tr2tls_thread_fallback); + return &tr2tls_thread_fallback; } struct tr2tls_thread_ctx *tr2tls_get_self(void) @@ -84,6 +98,11 @@ struct tr2tls_thread_ctx *tr2tls_get_self(void) return ctx; } +int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx) +{ + return ctx == &tr2tls_thread_fallback; +} + int tr2tls_is_main_thread(void) { if (!HAVE_THREADS) @@ -100,6 +119,9 @@ void tr2tls_unset_self(void) pthread_setspecific(tr2tls_key, NULL); + if (tr2tls_is_fallback(ctx)) + return; + free((char *)ctx->thread_name); free(ctx->array_us_start); free(ctx); @@ -111,30 +133,33 @@ void tr2tls_push_self(uint64_t us_now) uint64_t *new_array; size_t new_alloc; - if (ctx->nr_skipped_regions) { - ctx->nr_skipped_regions++; - return; - } - - if (ctx->nr_open_regions < ctx->alloc) + if (tr2tls_is_fallback(ctx)) return; - if (ctx->alloc > SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) { + if (ctx->nr_skipped_regions) { ctx->nr_skipped_regions++; return; } - new_alloc = ctx->alloc * 2; - new_array = realloc(ctx->array_us_start, - new_alloc * sizeof(*ctx->array_us_start)); - if (!new_array) { - ctx->nr_skipped_regions++; - return; + if (ctx->nr_open_regions >= ctx->alloc) { + if (ctx->alloc > + SIZE_MAX / (2 * sizeof(*ctx->array_us_start))) { + ctx->nr_skipped_regions++; + return; + } + new_alloc = ctx->alloc * 2; + + new_array = realloc(ctx->array_us_start, + new_alloc * sizeof(*ctx->array_us_start)); + if (!new_array) { + ctx->nr_skipped_regions++; + return; + } + + ctx->array_us_start = new_array; + ctx->alloc = new_alloc; } - ctx->array_us_start = new_array; - ctx->alloc = new_alloc; - ctx->array_us_start[ctx->nr_open_regions++] = us_now; } @@ -142,6 +167,9 @@ void tr2tls_pop_self(void) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); + if (tr2tls_is_fallback(ctx)) + return; + if (ctx->nr_skipped_regions) { ctx->nr_skipped_regions--; return; @@ -157,6 +185,9 @@ void tr2tls_pop_unwind_self(void) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); + if (tr2tls_is_fallback(ctx)) + return; + while (ctx->nr_open_regions > 1) tr2tls_pop_self(); } @@ -167,6 +198,8 @@ uint64_t tr2tls_region_elasped_self(uint64_t us) uint64_t us_start; ctx = tr2tls_get_self(); + if (tr2tls_is_fallback(ctx)) + return 0; if (ctx->nr_skipped_regions) return 0; if (!ctx->nr_open_regions) @@ -188,6 +221,10 @@ uint64_t tr2tls_absolute_elapsed(uint64_t us) static void tr2tls_key_destructor(void *payload) { struct tr2tls_thread_ctx *ctx = payload; + + if (tr2tls_is_fallback(ctx)) + return; + free((char *)ctx->thread_name); free(ctx->array_us_start); free(ctx); diff --git a/trace2/tr2_tls.h b/trace2/tr2_tls.h index c365017923601f..4a0969c014a0a9 100644 --- a/trace2/tr2_tls.h +++ b/trace2/tr2_tls.h @@ -54,6 +54,12 @@ struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_base_name, */ struct tr2tls_thread_ctx *tr2tls_get_self(void); +/* + * Return true if the context is the non-allocating fallback used after an + * allocation failure. Callers must not modify a fallback context. + */ +int tr2tls_is_fallback(const struct tr2tls_thread_ctx *ctx); + /* * return true if the current thread is the main thread. */ diff --git a/trace2/tr2_tmr.c b/trace2/tr2_tmr.c index a329c466b92a31..b3d26e2b316796 100644 --- a/trace2/tr2_tmr.c +++ b/trace2/tr2_tmr.c @@ -38,8 +38,11 @@ static struct tr2_timer_metadata tr2_timer_metadata[TRACE2_NUMBER_OF_TIMERS] = { void tr2_start_timer(enum trace2_timer_id tid) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); - struct tr2_timer *t = &ctx->timer_block.timer[tid]; + struct tr2_timer *t; + if (tr2tls_is_fallback(ctx)) + return; + t = &ctx->timer_block.timer[tid]; t->recursion_count++; if (t->recursion_count > 1) return; /* ignore recursive starts */ @@ -50,10 +53,13 @@ void tr2_start_timer(enum trace2_timer_id tid) void tr2_stop_timer(enum trace2_timer_id tid) { struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); - struct tr2_timer *t = &ctx->timer_block.timer[tid]; + struct tr2_timer *t; uint64_t ns_now; uint64_t ns_interval; + if (tr2tls_is_fallback(ctx)) + return; + t = &ctx->timer_block.timer[tid]; assert(t->recursion_count > 0); t->recursion_count--; @@ -91,6 +97,8 @@ void tr2_update_final_timers(void) struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); enum trace2_timer_id tid; + if (tr2tls_is_fallback(ctx)) + return; if (!ctx->used_any_timer) return; @@ -137,6 +145,8 @@ void tr2_emit_per_thread_timers(tr2_tgt_evt_timer_t *fn_apply) struct tr2tls_thread_ctx *ctx = tr2tls_get_self(); enum trace2_timer_id tid; + if (tr2tls_is_fallback(ctx)) + return; if (!ctx->used_any_per_thread_timer) return;