From: Johannes Schindelin <Johannes.Schindelin@gmx•de>
To: Adam Murray via GitGitGadget <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, Adam Murray <ad@canva•com>,
Adam Murray <ad@canva•com>
Subject: Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified
Date: Wed, 22 Jan 2025 11:11:14 +0100 (CET) [thread overview]
Message-ID: <c1398676-ddd7-c161-e114-2249316b5311@gmx.de> (raw)
In-Reply-To: <pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com>
Hi Adam,
On Fri, 10 Jan 2025, Adam Murray via GitGitGadget wrote:
> From: Adam Murray <ad@canva•com>
>
> 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 <ad@canva•com>
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 peff@peff•net
>
> 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 <ad@canva•com>
>
> @@ 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
>
>
next prev parent reply other threads:[~2025-01-22 10:11 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-07 0:04 [PATCH] trace2: prevent segfault on config collection where no value specified Adam Murray via GitGitGadget
2024-11-07 2:01 ` Jeff King
2024-11-07 3:02 ` Junio C Hamano
2025-01-10 7:28 ` [PATCH v2] " Adam Murray via GitGitGadget
2025-01-10 17:10 ` Junio C Hamano
2025-01-22 10:11 ` Johannes Schindelin [this message]
2025-01-22 18:12 ` Junio C Hamano
2025-01-22 18:18 ` Junio C Hamano
2025-01-23 17:01 ` D. Ben Knoble
2025-01-23 18:02 ` Junio C Hamano
2025-01-23 21:15 ` D. Ben Knoble
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c1398676-ddd7-c161-e114-2249316b5311@gmx.de \
--to=johannes.schindelin@gmx$(echo .)de \
--cc=ad@canva$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox