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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2025, 17:10 UTC
Message-ID: <xmqqed1a8uhu.fsf@gitster.g>
In-Reply-To: <pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com>

"Adam Murray via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adam Murray <ad@canva.com>
>
> When TRACE2 analytics is enabled, a git config option that has no value
> causes a segfault.

We often call this "valueless true syntax". It may techincally correct to say "has no value", but it is more friendly to readers if you said "a configuration variable that is set to 'true' with the valueless true syntax".

Show 25 quoted lines

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

OK, so for "valueless true", we do not get the "value" element in the json output, which makes sense. Don't we have documentation that explains what each element in the output means and when they are given? Shouldn't we update it?

Show 5 quoted lines

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

OK.

The input did not spell the "=value" part and said "[section] key" to mean that section.key is true, so we report that without "=value" part. This also makes sense.

Show 13 quoted lines

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

I am not versed well enough in tgt-parf output format to tell if this makes sense. We'd need somebody else to review this part.

Thanks.