| Message ID | pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] trace2: prevent segfault on config collection where no value specified | expand |
"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". > 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? > - 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. > 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.
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 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > This patch looks good to me! Thanks. As I punted on reviewing the tgt_perf part, it is very good to see somebody else step in to look it over. Will queue. >> 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 >> >>
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> >> This patch looks good to me! > > Thanks. As I punted on reviewing the tgt_perf part, it is very good > to see somebody else step in to look it over. > > Will queue. The test part used broken indentation and also the use of test_when_finished was careless, so I'll touch it up before queuing. No need to resend. Thanks.
On Wed, Jan 22, 2025 at 1:18 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > >> > >> This patch looks good to me! > > > > Thanks. As I punted on reviewing the tgt_perf part, it is very good > > to see somebody else step in to look it over. > > > > Will queue. > > The test part used broken indentation and also the use of > test_when_finished was careless, so I'll touch it up before queuing. > > No need to resend. > > Thanks. > I was curious what changed, so I found 792a3850fa (trace2: prevent segfault on config collection with valueless true, 2025-01-10) and I noticed this: - redacted = redact_arg(value); + redacted = value ? redact_arg(value): NULL; I think I expected (based on Documentation/CodingGuidelines) a space before the ternary conditional's colon. Then again, "git grep '?.*[^ ]:' *.[ch]" finds a few other cases with the unspaced style.
"D. Ben Knoble" <ben.knoble@gmail.com> writes: > I was curious what changed, so I found 792a3850fa (trace2: prevent > segfault on config collection with valueless true, 2025-01-10) and I > noticed this: > > - redacted = redact_arg(value); > + redacted = value ? redact_arg(value): NULL; Oh, I do not think I tweaked any of the code, other than fixing the new test that was not properly formatted. Mostly I touched up the log message. Will touch it up. Thanks for a careful reading.
On Thu, Jan 23, 2025 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote: > > "D. Ben Knoble" <ben.knoble@gmail.com> writes: > > > I was curious what changed, so I found 792a3850fa (trace2: prevent > > segfault on config collection with valueless true, 2025-01-10) and I > > noticed this: > > > > - redacted = redact_arg(value); > > + redacted = value ? redact_arg(value): NULL; > > Oh, I do not think I tweaked any of the code, other than fixing the > new test that was not properly formatted. Mostly I touched up the > log message. I could have been clearer—I think this was in the original. I just happened to notice it because I was curious about what touch-ups you performed :)
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,