Subject: Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified

From: Johannes Schindelin <[email-masked]>
Date: Wed, 22 Jan 2025 11:11:14 +0100 (CET)

Hi Adam,

On Fri, 10 Jan 2025, Adam Murray via GitGitGadget wrote:

> From: Adam Murray <[email-masked]>
>
> When TRACE2 analytics is enabled, a git config option that has no value
> causes a segfault.
>
> Steps to Reproduce
> GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*
> git -c status.relativePaths version
> Expected Result
> git version 2.46.0
> Actual Result
> zsh: segmentation fault GIT_TRACE2=true
>
> This adds checks to prevent the segfault and instead return
> an empty value.
>
> Signed-off-by: Adam Murray <[email-masked]>

This patch looks good to me!

Thank you for the fix,
Johannes

> ---
>     trace2: prevent segfault on config collection where no value specified
>
>     cc: Jeff King [email-masked]
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1814
>
> Range-diff vs v1:
>
>  1:  24ba9db7aa1 ! 1:  fd7bed52dda trace2: prevent segfault on config collection where no value specified
>      @@ Commit message
>           Actual Result
>           zsh: segmentation fault GIT_TRACE2=true
>
>      -    This adds a null check to prevent the segfault and instead return
>      -    the "empty config value" error.
>      +    This adds checks to prevent the segfault and instead return
>      +    an empty value.
>
>           Signed-off-by: Adam Murray <[email-masked]>
>
>      @@ t/t0210-trace2-normal.sh: test_expect_success 'bug messages followed by BUG() ar
>
>        ## trace2.c ##
>       @@ trace2.c: void trace2_def_param_fl(const char *file, int line, const char *param,
>      - 	int j;
>      - 	const char *redacted;
>      -
>      --	if (!trace2_enabled)
>      -+	if (!trace2_enabled || !value)
>      + 	if (!trace2_enabled)
>        		return;
>
>      - 	redacted = redact_arg(value);
>      +-	redacted = redact_arg(value);
>      ++	redacted = value ? redact_arg(value): NULL;
>      +
>      + 	for_each_wanted_builtin (j, tgt_j)
>      + 		if (tgt_j->pfn_param_fl)
>      +
>      + ## trace2/tr2_tgt_event.c ##
>      +@@ trace2/tr2_tgt_event.c: static void fn_param_fl(const char *file, int line, const char *param,
>      + 	event_fmt_prepare(event_name, file, line, NULL, &jw);
>      + 	jw_object_string(&jw, "scope", scope_name);
>      + 	jw_object_string(&jw, "param", param);
>      +-	jw_object_string(&jw, "value", value);
>      ++	if (value)
>      ++		jw_object_string(&jw, "value", value);
>      + 	jw_end(&jw);
>      +
>      + 	tr2_dst_write_line(&tr2dst_event, &jw.json);
>      +
>      + ## trace2/tr2_tgt_normal.c ##
>      +@@ trace2/tr2_tgt_normal.c: static void fn_param_fl(const char *file, int line, const char *param,
>      + 	enum config_scope scope = kvi->scope;
>      + 	const char *scope_name = config_scope_name(scope);
>      +
>      +-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
>      +-		    value);
>      ++	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
>      ++	if (value)
>      ++		strbuf_addf(&buf_payload, "=%s", value);
>      + 	normal_io_write_fl(file, line, &buf_payload);
>      + 	strbuf_release(&buf_payload);
>      + }
>      +
>      + ## trace2/tr2_tgt_perf.c ##
>      +@@ trace2/tr2_tgt_perf.c: static void fn_param_fl(const char *file, int line, const char *param,
>      + 	struct strbuf scope_payload = STRBUF_INIT;
>      + 	enum config_scope scope = kvi->scope;
>      + 	const char *scope_name = config_scope_name(scope);
>      +-
>      +-	strbuf_addf(&buf_payload, "%s:%s", param, value);
>      ++	strbuf_addstr(&buf_payload, param);
>      ++	if (value)
>      ++		strbuf_addf(&buf_payload, ":%s", value);
>      + 	strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
>      +
>      + 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,
>
>
>  t/t0210-trace2-normal.sh | 8 ++++++++
>  trace2.c                 | 2 +-
>  trace2/tr2_tgt_event.c   | 3 ++-
>  trace2/tr2_tgt_normal.c  | 5 +++--
>  trace2/tr2_tgt_perf.c    | 5 +++--
>  5 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index b9adc94aab4..4047ab562a4 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'empty configuration values are handled' '
> +  test_when_finished "rm trace2.normal actual expect" &&
> +  echo >expect &&
> +  GIT_TRACE2="$(pwd)/trace2.normal"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \
> +	  git -c foo.empty config foo.empty >actual &&
> +	test_cmp expect actual
> +'
> +
>  sane_unset GIT_TRACE2_BRIEF
>
>  # Now test without environment variables and get all Trace2 settings
> diff --git a/trace2.c b/trace2.c
> index f894532d053..49e7d1db88f 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
>  	if (!trace2_enabled)
>  		return;
>
> -	redacted = redact_arg(value);
> +	redacted = value ? redact_arg(value): NULL;
>
>  	for_each_wanted_builtin (j, tgt_j)
>  		if (tgt_j->pfn_param_fl)
> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
> index 45b0850a5ec..8e09485c83c 100644
> --- a/trace2/tr2_tgt_event.c
> +++ b/trace2/tr2_tgt_event.c
> @@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,
>  	event_fmt_prepare(event_name, file, line, NULL, &jw);
>  	jw_object_string(&jw, "scope", scope_name);
>  	jw_object_string(&jw, "param", param);
> -	jw_object_string(&jw, "value", value);
> +	if (value)
> +		jw_object_string(&jw, "value", value);
>  	jw_end(&jw);
>
>  	tr2_dst_write_line(&tr2dst_event, &jw.json);
> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
> index baef48aa698..924736ab360 100644
> --- a/trace2/tr2_tgt_normal.c
> +++ b/trace2/tr2_tgt_normal.c
> @@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
>  	enum config_scope scope = kvi->scope;
>  	const char *scope_name = config_scope_name(scope);
>
> -	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
> -		    value);
> +	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
> +	if (value)
> +		strbuf_addf(&buf_payload, "=%s", value);
>  	normal_io_write_fl(file, line, &buf_payload);
>  	strbuf_release(&buf_payload);
>  }
> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
> index a6f9a8a193e..19ae7433ef8 100644
> --- a/trace2/tr2_tgt_perf.c
> +++ b/trace2/tr2_tgt_perf.c
> @@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
>  	struct strbuf scope_payload = STRBUF_INIT;
>  	enum config_scope scope = kvi->scope;
>  	const char *scope_name = config_scope_name(scope);
> -
> -	strbuf_addf(&buf_payload, "%s:%s", param, value);
> +	strbuf_addstr(&buf_payload, param);
> +	if (value)
> +		strbuf_addf(&buf_payload, ":%s", value);
>  	strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
>
>  	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,
>
> base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772
> --
> gitgitgadget
>
>