| Message ID | pull.1814.git.1730937889182.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | trace2: prevent segfault on config collection where no value specified | expand |
On Thu, Nov 07, 2024 at 12:04:48AM +0000, Adam Murray via GitGitGadget wrote: > 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 a null check to prevent the segfault and instead return > the "empty config value" error. We definitely should deal with the NULL here, but I'm not sure that returning an error is correct. A value-less config like this is a synonym for "true". If the point of this code is to dump a trace of config settings, then by returning without printing anything, we're misleading the user. I.e., doing this, with an explicit value for the config option: GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version should (and does) show: 20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths=true If we swap that our for "-c status.relativePaths", then the outcome is the same: we've turned on that config option. But with your patch, the trace won't mention it at all. > diff --git a/trace2.c b/trace2.c > index f894532d053..5df43478b8f 100644 > --- a/trace2.c > +++ b/trace2.c > @@ -759,7 +759,7 @@ 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) > return; > > redacted = redact_arg(value); So here I think we need to either: 1. Just quietly substitute "true" for the value. For a bool, the two are equivalent, and this is probably an acceptable fiction for a trace to show. For a non-bool (e.g., something like "author.name"), though, it's an error, and the trace is somewhat misleading. 2. Put in some special marker for the NULL value. Something like "(null)" works, but it's ambiguous with a config of the same value (which obviously you wouldn't expect in normal use, but since the point of tracing is often to debug, I could see it being misleading). All of this is made harder by the fact that there are multiple output targets. So you'd have to pass the NULL down to them and let them handle it. Something like: diff --git a/trace2.c b/trace2.c index 5df43478b8..e67edf4b1b 100644 --- a/trace2.c +++ b/trace2.c @@ -759,10 +759,10 @@ void trace2_def_param_fl(const char *file, int line, const char *param, int j; const char *redacted; - if (!trace2_enabled || !value) + 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_normal.c b/trace2/tr2_tgt_normal.c index baef48aa69..924736ab36 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); } but you'd need to do the same for each target implementation. -Peff
Jeff King <peff@peff.net> writes: > I.e., doing this, with an explicit value for the config option: > > GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version > > should (and does) show: > > 20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths=true > > If we swap that our for "-c status.relativePaths", then the outcome is > the same: we've turned on that config option. But with your patch, the > trace won't mention it at all. which may be improvement, but ideally, the "valueless truth" case should be given differently, perhaps like 20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths to allow showing what exactly the system has seen. After all, trace output is often used for debugging, and it is not unusual for a buggy code path to behave on explicit truth and valueless truth differently. > So here I think we need to either: > > 1. Just quietly substitute "true" for the value. For a bool, the two > are equivalent, and this is probably an acceptable fiction for a > trace to show. For a non-bool (e.g., something like "author.name"), > though, it's an error, and the trace is somewhat misleading. > > 2. Put in some special marker for the NULL value. Something like > "(null)" works, but it's ambiguous with a config of the same value > (which obviously you wouldn't expect in normal use, but since the > point of tracing is often to debug, I could see it being > misleading). > > All of this is made harder by the fact that there are multiple output > targets. So you'd have to pass the NULL down to them and let them handle > it. Something like: > ... > diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c > index baef48aa69..924736ab36 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); Yes, exactly. > normal_io_write_fl(file, line, &buf_payload); > strbuf_release(&buf_payload); > } > > but you'd need to do the same for each target implementation. Thanks.
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..5df43478b8f 100644 --- a/trace2.c +++ b/trace2.c @@ -759,7 +759,7 @@ 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) return; redacted = redact_arg(value);