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
c483a4b
banned-die: create header for banning of functions
derrickstolee Aug 2, 2026
754fffb
trace2: tolerate failed timestamp formatting
derrickstolee Jul 13, 2026
87d3f1b
trace2: remove use of xstrdup()
derrickstolee Aug 2, 2026
5bf6ab9
trace2: remove use of ALLOC_ARRAY()
derrickstolee Aug 2, 2026
3e419c5
trace2: remove use of xstrfmt()
derrickstolee Aug 2, 2026
ccd284f
trace2: remove use of ALLOC_GROW()
derrickstolee Aug 2, 2026
fa10e8d
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 31, 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.

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

On 8/25/2026 4:34 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I'd be curious if it would satisfy two directions for those cases:

1. Help declare a subsystem is free of these globals and thus is
   ready for multi-hash or multi-repo handling.

2. Help declare a subsystem is _not_ free of these globals and thus
   should not be _reintroduced_ into a subsystem that was declared
   clean.

We'd need both, in general. And we'd need to continue expanding the
banned-*.h files. I am curious as to whether there are static tools
that could assist with this.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Aug 31, 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.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Tue, Aug 25, 2026 at 01:34:53PM -0700, Junio C Hamano wrote:
> "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?

Don't we already do this? If `USE_THE_REPOSITORY_VARIABLE` is not
defined then we hide several function declarations where we know that
they depend on `the_repository`. It's not perfect as we still expose
functions that do rely on it implicitly, but it's easy to remove more
function declarations over time by just adding another ifdef.

Maybe we should follow a similar approach with functions that die?

Patrick

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

@gitgitgadget gitgitgadget Bot Aug 31, 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.

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

On 8/25/2026 6:14 PM, Elijah Newren wrote:
> 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)?

Oops. Yes, a mistake during a rebase. 
>> +
>> +#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?

There is a risk. The "must be last" part is documented in the
header, but maybe it should be in a comment here, too.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Aug 27, 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.

Jeff King wrote on the Git mailing list (how to reply to this email):

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

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

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

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

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

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

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

-Peff

@gitgitgadget gitgitgadget Bot Aug 31, 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.

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

On 8/27/2026 1:10 AM, Jeff King wrote:
> On Tue, Aug 25, 2026 at 06:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> We have universally-banned functions listed in banned.h since
>> c8af66ab8ad (automatically ban strcpy(), 2018-07-26), but some layers of
>> the code should be more strict than others.
>>
>> One such example is the trace2 API which runs during atexit() and can
>> prove to cause die()-handler recursion problems if it calls die().
>>
>> Create a new banned-die.h header file that will ban some Git methods
>> that call die(). Include that in all trace2 API implementation files.
>> This currently only bans die() itself, and that was already not used.
> 
> There's a subtle but big difference between the universal code bans in
> banned.h and this banned-die.h. In the former case we are deciding
> strcpy() is unfit for our code base and outlawing it everywhere. The
> potential problem is in the source code, so catching it while compiling
> the source code is OK.
> 
> But we are not doing that with die(). It is a perfectly OK function in
> general, but we do not want to ever trigger its runtime effects from
> certain code paths. Banning it from being called from those code paths
> can catch _some_ instances, but not any transitive calls. If we call
> foo(), it may call die() itself, and we would not want to ban foo() from
> doing so. And recursively for functions called by foo() and so on.

Yes, this makes it tricky to be 100% sure without some kind of static
analysis.

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

This does have some benefit that we can gradually remove these
transitive callers in the multi-commit series. But it's unsatisfying
as a full protection in the end.

> I think that's _probably_ the best we can do in practice. I think the
> framing above suggests that we could approach the problem more directly
> with a runtime flag: when we enter those code paths, set a flag to avoid
> the unwanted behavior, and have the low-level code respect that. But
> die() is a special case here, because we'd want to suppress its
> no-return behavior. And its callers are not prepared for die() to
> suddenly start returning because of some global flag.
> 
> So I think the whack-a-mole is the best we can do. But I would not want
> to see this strategy extended to other areas. In most cases some kind of
> runtime support is probably a better solution.
The other alternative that we could consider is to reorganize the
codebase in such a way that certain sections of code don't have
access to headers that could lead to die() or other "higher" methods
that are acceptable for user-facing processes but are best to avoid
in library APIs. Even then, we'd need some checks at compile time to
avoid crossing boundaries.

I don't think such a reorganization is desirable overall, because
that will be very disruptive to the project and file history.

Having some amount of protection through this header gives us a
mechanism to demonstrate and enforce some protection.

Thanks,
-Stolee

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

@gitgitgadget gitgitgadget Bot Aug 31, 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.

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

On 8/25/2026 6:14 PM, Elijah Newren wrote:
> 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)

Thanks for the careful read! It's particularly important that we
don't suggest that the config value is bad because we couldn't
allocate memory.

> 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.

Yes, I believe you are correct. We should return 0 to terminate
early without a failure.

That said, I think that the die() in the config code will remain a
"safe" place to die(), as we won't re-trigger this config-parsing
code during any tracing of that die() message. But it's best to be
safe and have the tracing continue to be "best effort" when system
calls fail.

Thanks,
-Stolee

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

@gitgitgadget gitgitgadget Bot Aug 31, 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.

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

On 8/25/2026 6:36 PM, Junio C Hamano wrote:
> 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?
I should have been more careful and realized that we don't have any
tests that cover this logic.

We have tests for ":<redacted>" in pkt-line output, but not for the
trace2 version.

Thanks,
-Stolee

#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 */
12 changes: 8 additions & 4 deletions t/t0212-trace2-event.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,8 @@ test_expect_success 'unsafe URLs are redacted by default in cmd_start events' '

GIT_TRACE2_EVENT="$(pwd)/trace.event" \
test-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 &&
test_grep ! user:pwd trace.event
test_grep ! user:pwd trace.event &&
test_grep "user:<REDACTED>@example.com/" trace.event
'

test_expect_success 'unsafe URLs are redacted by default in child_start events' '
Expand All @@ -341,7 +342,8 @@ test_expect_success 'unsafe URLs are redacted by default in child_start events'

GIT_TRACE2_EVENT="$(pwd)/trace.event" \
test-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 &&
test_grep ! user:pwd trace.event
test_grep ! user:pwd trace.event &&
test_grep "user:<REDACTED>@example.com/" trace.event
'

test_expect_success 'unsafe URLs are redacted by default in exec events' '
Expand All @@ -350,7 +352,8 @@ test_expect_success 'unsafe URLs are redacted by default in exec events' '

GIT_TRACE2_EVENT="$(pwd)/trace.event" \
test-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 &&
test_grep ! user:pwd trace.event
test_grep ! user:pwd trace.event &&
test_grep "user:<REDACTED>@example.com/" trace.event
'

test_expect_success 'unsafe URLs are redacted by default in def_param events' '
Expand All @@ -359,7 +362,8 @@ test_expect_success 'unsafe URLs are redacted by default in def_param events' '

GIT_TRACE2_EVENT="$(pwd)/trace.event" \
test-tool trace2 303redact_def_param url https://user:pwd@example.com/ &&
test_grep ! user:pwd trace.event
test_grep ! user:pwd trace.event &&
test_grep "user:<REDACTED>@example.com/" trace.event
'

test_done
52 changes: 48 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,8 @@
#include "trace2/tr2_tgt.h"
#include "trace2/tr2_tls.h"
#include "trace2/tr2_tmr.h"
/* banned-die must be last. */
#include "banned-die.h"

static int trace2_enabled;
static int trace2_redact = 1;
Expand Down Expand Up @@ -259,7 +261,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 +279,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) ||
unsigned_add_overflows(prefix_len + suffix_len + redact_len, 1))
return NULL;

redacted_len = prefix_len + suffix_len + redact_len + 1;

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

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

/*
Expand All @@ -299,19 +322,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 +380,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 +550,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 +568,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 +663,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
2 changes: 2 additions & 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,8 @@
#include "trace2/tr2_cfg.h"
#include "trace2/tr2_sysenv.h"
#include "wildmatch.h"
/* banned-die must be last. */
#include "banned-die.h"

static struct string_list tr2_cfg_patterns = STRING_LIST_INIT_DUP;
static int tr2_cfg_loaded;
Expand Down
2 changes: 2 additions & 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,8 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "trace2/tr2_cmd_name.h"
/* banned-die must be last. */
#include "banned-die.h"

#define TR2_ENVVAR_PARENT_NAME "GIT_TRACE2_PARENT_NAME"

Expand Down
12 changes: 11 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,8 @@
#include "trace2/tr2_tgt.h"
#include "trace2/tr2_tls.h"
#include "trace2/tr2_ctr.h"
/* banned-die must be last. */
#include "banned-die.h"

/*
* A global counter block to aggregate values from the partial sums
Expand Down Expand Up @@ -53,7 +55,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 +73,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 +96,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
2 changes: 2 additions & 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,8 @@
#include "trace2/tr2_dst.h"
#include "trace2/tr2_sid.h"
#include "trace2/tr2_sysenv.h"
/* banned-die must be last. */
#include "banned-die.h"

/*
* How many attempts we will make at creating an automatically-named trace file.
Expand Down
2 changes: 2 additions & 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,8 @@
#include "strbuf.h"
#include "trace2/tr2_tbuf.h"
#include "trace2/tr2_sid.h"
/* banned-die must be last. */
#include "banned-die.h"

#define TR2_ENVVAR_PARENT_SID "GIT_TRACE2_PARENT_SID"

Expand Down
8 changes: 6 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,8 @@
#include "config.h"
#include "dir.h"
#include "tr2_sysenv.h"
/* banned-die must be last. */
#include "banned-die.h"

/*
* Each entry represents a trace2 setting.
Expand Down Expand Up @@ -73,7 +75,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 0;
return 0;
}
}
Expand Down Expand Up @@ -109,7 +113,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
51 changes: 36 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,68 @@
#include "git-compat-util.h"
#include "tr2_tbuf.h"
/* banned-die must be last. */
#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));
}
}
2 changes: 2 additions & 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,8 @@
#include "trace2/tr2_tgt.h"
#include "trace2/tr2_tls.h"
#include "trace2/tr2_tmr.h"
/* banned-die must be last. */
#include "banned-die.h"

static struct tr2_dst tr2dst_event = {
.sysenv_var = TR2_SYSENV_EVENT,
Expand Down
2 changes: 2 additions & 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,8 @@
#include "trace2/tr2_tgt.h"
#include "trace2/tr2_tls.h"
#include "trace2/tr2_tmr.h"
/* banned-die must be last. */
#include "banned-die.h"

static struct tr2_dst tr2dst_normal = {
.sysenv_var = TR2_SYSENV_NORMAL,
Expand Down
Loading
Loading