Hi Adam, On Fri, 10 Jan 2025, Adam Murray via GitGitGadget wrote: > From: Adam Murray <ad@xxxxxxxxx> > > 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@xxxxxxxxx> 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@xxxxxxxx > > 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@xxxxxxxxx> > > @@ 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 > >
