Skip to content

trace2: stop allowing die() #2178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
derrickstolee wants to merge 7 commits into
base: master
Choose a base branch
from
Open

trace2: stop allowing die() #2178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8463471
banned-die: create header for banning of functions
derrickstolee Aug 2, 2026
bd45f46
trace2: tolerate failed timestamp formatting
derrickstolee Jul 13, 2026
ec447a6
trace2: remove use of xstrdup()
derrickstolee Aug 2, 2026
db6858d
trace2: remove use of ALLOC_ARRAY()
derrickstolee Aug 2, 2026
7f0bb40
trace2: remove use of xstrfmt()
derrickstolee Aug 2, 2026
120cf19
trace2: remove use of ALLOC_GROW()
derrickstolee Aug 2, 2026
c8fc195
trace2: remove use of xcalloc()
derrickstolee Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions banned-die.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#ifndef BANNED_DIE_H

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

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

#undef xsnprintf

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

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

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

#define xsnprintf(...) BANNED(xsnprintf)

#undef xstrdup

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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


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

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Elijah Newren <newren@gmail.com> writes:

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

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

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

#define xstrdup(str) BANNED(xstrdup)

#undef xcalloc
#define xcalloc(nmemb, size) BANNED(xcalloc)

#undef xstrfmt
#define xstrfmt(...) BANNED(xstrfmt)

#undef ALLOC_ARRAY

@gitgitgadget gitgitgadget Bot Aug 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Checking out this commit and running

   GIT_TRACE2_PERF=1 ./bin-wrappers/git status

dies with

   no open regions in thread 'main'

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

#define ALLOC_ARRAY(x, alloc) BANNED(ALLOC_ARRAY)

#undef ALLOC_GROW
#define ALLOC_GROW(x, nr, alloc) BANNED(ALLOC_GROW)

#endif /* BANNED_DIE_H */
51 changes: 47 additions & 4 deletions trace2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -259,7 +260,10 @@ int trace2_is_enabled(void)
static const char *redact_arg(const char *arg)
{
const char *p, *colon;
const char *redact = ":<REDACTED>";
char *redacted;
size_t at;
size_t prefix_len, suffix_len, redacted_len, redact_len;

if (!trace2_redact ||
(!skip_prefix(arg, "https://", &p) &&
Expand All @@ -274,7 +278,25 @@ static const char *redact_arg(const char *arg)
if (!colon)
return arg;

return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
redact_len = strlen(redact);
prefix_len = colon - arg;
suffix_len = strlen(p + at);

if (unsigned_add_overflows(prefix_len, suffix_len) ||
unsigned_add_overflows(prefix_len + suffix_len, redact_len))
return NULL;

redacted_len = prefix_len + suffix_len + redact_len;

redacted = malloc(redacted_len);
if (!redacted)
return NULL;

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

/*
Expand All @@ -299,19 +321,32 @@ 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 */

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++)
ret[j] = argv[j];
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;
Expand Down Expand Up @@ -344,6 +379,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)
Expand Down Expand Up @@ -512,6 +549,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;
Expand All @@ -529,7 +567,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)
Expand Down Expand Up @@ -621,6 +662,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)
Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_cfg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_cmd_name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

Expand Down
11 changes: 10 additions & 1 deletion trace2/tr2_ctr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -53,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;

Expand All @@ -67,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;

Expand All @@ -88,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;

Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_dst.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_sid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
7 changes: 5 additions & 2 deletions trace2/tr2_sysenv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "config.h"
#include "dir.h"
#include "tr2_sysenv.h"
#include "banned-die.h"

/*
* Each entry represents a trace2 setting.
Expand Down Expand Up @@ -73,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;
}
}
Expand Down Expand Up @@ -109,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;
}
Expand Down
50 changes: 35 additions & 15 deletions trace2/tr2_tbuf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,67 @@
#include "git-compat-util.h"
#include "tr2_tbuf.h"
#include "banned-die.h"

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));
}
}
1 change: 1 addition & 0 deletions trace2/tr2_tgt_event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_tgt_normal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions trace2/tr2_tgt_perf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading