{"thread":{"id":"62462","subject":"[PATCH] trace2: prevent segfault on config collection where no value specified","startedAt":"2024-11-07T00:04:52Z","lastAt":"2025-01-23T21:15:25Z","messageCount":11,"participants":["Adam Murray via GitGitGadget","Jeff King","Junio C Hamano","Johannes Schindelin","D. Ben Knoble"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"506764","messageId":"pull.1814.git.1730937889182.gitgitgadget@gmail.com","threadId":"62462","inReplyTo":null,"subject":"[PATCH] trace2: prevent segfault on config collection where no value specified","fromName":"Adam Murray via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-07T00:04:48Z","receivedAt":"2024-11-07T00:04:52Z","isPatch":true,"sender":{"key":"name:Adam Murray","avatar":null},"body":"From: Adam Murray <ad@canva.com>\n\nWhen TRACE2 analytics is enabled, a git config option that has no value\ncauses a segfault.\n\nSteps to Reproduce\nGIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*\ngit -c status.relativePaths version\nExpected Result\ngit version 2.46.0\nActual Result\nzsh: segmentation fault GIT_TRACE2=true\n\nThis adds a null check to prevent the segfault and instead return\nthe \"empty config value\" error.\n\nSigned-off-by: Adam Murray <ad@canva.com>\n---\n    trace2: prevent segfault on config collection where no value specified\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1814\n\n t/t0210-trace2-normal.sh | 8 ++++++++\n trace2.c                 | 2 +-\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\nindex b9adc94aab4..4047ab562a4 100755\n--- a/t/t0210-trace2-normal.sh\n+++ b/t/t0210-trace2-normal.sh\n@@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'empty configuration values are handled' '\n+  test_when_finished \"rm trace2.normal actual expect\" &&\n+  echo >expect &&\n+  GIT_TRACE2=\"$(pwd)/trace2.normal\"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \\\n+\t  git -c foo.empty config foo.empty >actual &&\n+\ttest_cmp expect actual\n+'\n+\n sane_unset GIT_TRACE2_BRIEF\n \n # Now test without environment variables and get all Trace2 settings\ndiff --git a/trace2.c b/trace2.c\nindex f894532d053..5df43478b8f 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -759,7 +759,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n \tint j;\n \tconst char *redacted;\n \n-\tif (!trace2_enabled)\n+\tif (!trace2_enabled || !value)\n \t\treturn;\n \n \tredacted = redact_arg(value);\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n-- \ngitgitgadget\n"},{"id":"506770","messageId":"20241107020100.GC961214@coredump.intra.peff.net","threadId":"62462","inReplyTo":"pull.1814.git.1730937889182.gitgitgadget@gmail.com","subject":"Re: [PATCH] trace2: prevent segfault on config collection where no value specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-07T02:01:00Z","receivedAt":"2024-11-07T02:01:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 12:04:48AM +0000, Adam Murray via GitGitGadget wrote:\n\n> When TRACE2 analytics is enabled, a git config option that has no value\n> causes a segfault.\n> \n> Steps to Reproduce\n> GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*\n> git -c status.relativePaths version\n> Expected Result\n> git version 2.46.0\n> Actual Result\n> zsh: segmentation fault GIT_TRACE2=true\n> \n> This adds a null check to prevent the segfault and instead return\n> the \"empty config value\" error.\n\nWe definitely should deal with the NULL here, but I'm not sure that\nreturning an error is correct. A value-less config like this is a\nsynonym for \"true\". If the point of this code is to dump a trace of\nconfig settings, then by returning without printing anything, we're\nmisleading the user.\n\nI.e., doing this, with an explicit value for the config option:\n\n  GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version\n\nshould (and does) show:\n\n  20:48:11.662470 trace2.c:437                      def_param scope:command status.relativepaths=true\n\nIf we swap that our for \"-c status.relativePaths\", then the outcome is\nthe same: we've turned on that config option. But with your patch, the\ntrace won't mention it at all.\n\n> diff --git a/trace2.c b/trace2.c\n> index f894532d053..5df43478b8f 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -759,7 +759,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n>  \tint j;\n>  \tconst char *redacted;\n>  \n> -\tif (!trace2_enabled)\n> +\tif (!trace2_enabled || !value)\n>  \t\treturn;\n>  \n>  \tredacted = redact_arg(value);\n\nSo here I think we need to either:\n\n  1. Just quietly substitute \"true\" for the value. For a bool, the two\n     are equivalent, and this is probably an acceptable fiction for a\n     trace to show. For a non-bool (e.g., something like \"author.name\"),\n     though, it's an error, and the trace is somewhat misleading.\n\n  2. Put in some special marker for the NULL value. Something like\n     \"(null)\" works, but it's ambiguous with a config of the same value\n     (which obviously you wouldn't expect in normal use, but since the\n     point of tracing is often to debug, I could see it being\n     misleading).\n\nAll of this is made harder by the fact that there are multiple output\ntargets. So you'd have to pass the NULL down to them and let them handle\nit. Something like:\n\ndiff --git a/trace2.c b/trace2.c\nindex 5df43478b8..e67edf4b1b 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -759,10 +759,10 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n \tint j;\n \tconst char *redacted;\n \n-\tif (!trace2_enabled || !value)\n+\tif (!trace2_enabled)\n \t\treturn;\n \n-\tredacted = redact_arg(value);\n+\tredacted = value ? redact_arg(value) : NULL;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_param_fl)\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex baef48aa69..924736ab36 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n \tenum config_scope scope = kvi->scope;\n \tconst char *scope_name = config_scope_name(scope);\n \n-\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n-\t\t    value);\n+\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n+\tif (value)\n+\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n \tnormal_io_write_fl(file, line, &buf_payload);\n \tstrbuf_release(&buf_payload);\n }\n\nbut you'd need to do the same for each target implementation.\n\n-Peff\n"},{"id":"506776","messageId":"xmqqikszvj8o.fsf@gitster.g","threadId":"62462","inReplyTo":"20241107020100.GC961214@coredump.intra.peff.net","subject":"Re: [PATCH] trace2: prevent segfault on config collection where no value specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-07T03:02:47Z","receivedAt":"2024-11-07T03:02:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I.e., doing this, with an explicit value for the config option:\n>\n>   GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version\n>\n> should (and does) show:\n>\n>   20:48:11.662470 trace2.c:437                      def_param scope:command status.relativepaths=true\n>\n> If we swap that our for \"-c status.relativePaths\", then the outcome is\n> the same: we've turned on that config option. But with your patch, the\n> trace won't mention it at all.\n\nwhich may be improvement, but ideally, the \"valueless truth\" case\nshould be given differently, perhaps like \n\n   20:48:11.662470 trace2.c:437                      def_param scope:command status.relativepaths\n\nto allow showing what exactly the system has seen.  After all, trace\noutput is often used for debugging, and it is not unusual for a\nbuggy code path to behave on explicit truth and valueless truth\ndifferently.\n\n> So here I think we need to either:\n>\n>   1. Just quietly substitute \"true\" for the value. For a bool, the two\n>      are equivalent, and this is probably an acceptable fiction for a\n>      trace to show. For a non-bool (e.g., something like \"author.name\"),\n>      though, it's an error, and the trace is somewhat misleading.\n>\n>   2. Put in some special marker for the NULL value. Something like\n>      \"(null)\" works, but it's ambiguous with a config of the same value\n>      (which obviously you wouldn't expect in normal use, but since the\n>      point of tracing is often to debug, I could see it being\n>      misleading).\n>\n> All of this is made harder by the fact that there are multiple output\n> targets. So you'd have to pass the NULL down to them and let them handle\n> it. Something like:\n> ...\n> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\n> index baef48aa69..924736ab36 100644\n> --- a/trace2/tr2_tgt_normal.c\n> +++ b/trace2/tr2_tgt_normal.c\n> @@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tenum config_scope scope = kvi->scope;\n>  \tconst char *scope_name = config_scope_name(scope);\n>  \n> -\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n> -\t\t    value);\n> +\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n> +\tif (value)\n> +\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n\nYes, exactly.\n\n>  \tnormal_io_write_fl(file, line, &buf_payload);\n>  \tstrbuf_release(&buf_payload);\n>  }\n>\n> but you'd need to do the same for each target implementation.\n\nThanks.\n"},{"id":"510278","messageId":"pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com","threadId":"62462","inReplyTo":"pull.1814.git.1730937889182.gitgitgadget@gmail.com","subject":"[PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Adam Murray via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-10T07:28:20Z","receivedAt":"2025-01-10T07:28:24Z","isPatch":true,"sender":{"key":"name:Adam Murray","avatar":null},"body":"From: Adam Murray <ad@canva.com>\n\nWhen TRACE2 analytics is enabled, a git config option that has no value\ncauses a segfault.\n\nSteps to Reproduce\nGIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*\ngit -c status.relativePaths version\nExpected Result\ngit version 2.46.0\nActual Result\nzsh: segmentation fault GIT_TRACE2=true\n\nThis adds checks to prevent the segfault and instead return\nan empty value.\n\nSigned-off-by: Adam Murray <ad@canva.com>\n---\n    trace2: prevent segfault on config collection where no value specified\n    \n    cc: Jeff King peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1814\n\nRange-diff vs v1:\n\n 1:  24ba9db7aa1 ! 1:  fd7bed52dda trace2: prevent segfault on config collection where no value specified\n     @@ Commit message\n          Actual Result\n          zsh: segmentation fault GIT_TRACE2=true\n      \n     -    This adds a null check to prevent the segfault and instead return\n     -    the \"empty config value\" error.\n     +    This adds checks to prevent the segfault and instead return\n     +    an empty value.\n      \n          Signed-off-by: Adam Murray <ad@canva.com>\n      \n     @@ t/t0210-trace2-normal.sh: test_expect_success 'bug messages followed by BUG() ar\n      \n       ## trace2.c ##\n      @@ trace2.c: void trace2_def_param_fl(const char *file, int line, const char *param,\n     - \tint j;\n     - \tconst char *redacted;\n     - \n     --\tif (!trace2_enabled)\n     -+\tif (!trace2_enabled || !value)\n     + \tif (!trace2_enabled)\n       \t\treturn;\n       \n     - \tredacted = redact_arg(value);\n     +-\tredacted = redact_arg(value);\n     ++\tredacted = value ? redact_arg(value): NULL;\n     + \n     + \tfor_each_wanted_builtin (j, tgt_j)\n     + \t\tif (tgt_j->pfn_param_fl)\n     +\n     + ## trace2/tr2_tgt_event.c ##\n     +@@ trace2/tr2_tgt_event.c: static void fn_param_fl(const char *file, int line, const char *param,\n     + \tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n     + \tjw_object_string(&jw, \"scope\", scope_name);\n     + \tjw_object_string(&jw, \"param\", param);\n     +-\tjw_object_string(&jw, \"value\", value);\n     ++\tif (value)\n     ++\t\tjw_object_string(&jw, \"value\", value);\n     + \tjw_end(&jw);\n     + \n     + \ttr2_dst_write_line(&tr2dst_event, &jw.json);\n     +\n     + ## trace2/tr2_tgt_normal.c ##\n     +@@ trace2/tr2_tgt_normal.c: static void fn_param_fl(const char *file, int line, const char *param,\n     + \tenum config_scope scope = kvi->scope;\n     + \tconst char *scope_name = config_scope_name(scope);\n     + \n     +-\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n     +-\t\t    value);\n     ++\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n     ++\tif (value)\n     ++\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n     + \tnormal_io_write_fl(file, line, &buf_payload);\n     + \tstrbuf_release(&buf_payload);\n     + }\n     +\n     + ## trace2/tr2_tgt_perf.c ##\n     +@@ trace2/tr2_tgt_perf.c: static void fn_param_fl(const char *file, int line, const char *param,\n     + \tstruct strbuf scope_payload = STRBUF_INIT;\n     + \tenum config_scope scope = kvi->scope;\n     + \tconst char *scope_name = config_scope_name(scope);\n     +-\n     +-\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n     ++\tstrbuf_addstr(&buf_payload, param);\n     ++\tif (value)\n     ++\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n     + \tstrbuf_addf(&scope_payload, \"%s:%s\", \"scope\", scope_name);\n     + \n     + \tperf_io_write_fl(file, line, event_name, NULL, NULL, NULL,\n\n\n t/t0210-trace2-normal.sh | 8 ++++++++\n trace2.c                 | 2 +-\n trace2/tr2_tgt_event.c   | 3 ++-\n trace2/tr2_tgt_normal.c  | 5 +++--\n trace2/tr2_tgt_perf.c    | 5 +++--\n 5 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\nindex b9adc94aab4..4047ab562a4 100755\n--- a/t/t0210-trace2-normal.sh\n+++ b/t/t0210-trace2-normal.sh\n@@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'empty configuration values are handled' '\n+  test_when_finished \"rm trace2.normal actual expect\" &&\n+  echo >expect &&\n+  GIT_TRACE2=\"$(pwd)/trace2.normal\"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \\\n+\t  git -c foo.empty config foo.empty >actual &&\n+\ttest_cmp expect actual\n+'\n+\n sane_unset GIT_TRACE2_BRIEF\n \n # Now test without environment variables and get all Trace2 settings\ndiff --git a/trace2.c b/trace2.c\nindex f894532d053..49e7d1db88f 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n \tif (!trace2_enabled)\n \t\treturn;\n \n-\tredacted = redact_arg(value);\n+\tredacted = value ? redact_arg(value): NULL;\n \n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_param_fl)\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 45b0850a5ec..8e09485c83c 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,\n \tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n \tjw_object_string(&jw, \"scope\", scope_name);\n \tjw_object_string(&jw, \"param\", param);\n-\tjw_object_string(&jw, \"value\", value);\n+\tif (value)\n+\t\tjw_object_string(&jw, \"value\", value);\n \tjw_end(&jw);\n \n \ttr2_dst_write_line(&tr2dst_event, &jw.json);\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex baef48aa698..924736ab360 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n \tenum config_scope scope = kvi->scope;\n \tconst char *scope_name = config_scope_name(scope);\n \n-\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n-\t\t    value);\n+\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n+\tif (value)\n+\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n \tnormal_io_write_fl(file, line, &buf_payload);\n \tstrbuf_release(&buf_payload);\n }\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex a6f9a8a193e..19ae7433ef8 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n \tstruct strbuf scope_payload = STRBUF_INIT;\n \tenum config_scope scope = kvi->scope;\n \tconst char *scope_name = config_scope_name(scope);\n-\n-\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n+\tstrbuf_addstr(&buf_payload, param);\n+\tif (value)\n+\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n \tstrbuf_addf(&scope_payload, \"%s:%s\", \"scope\", scope_name);\n \n \tperf_io_write_fl(file, line, event_name, NULL, NULL, NULL,\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n-- \ngitgitgadget\n"},{"id":"510332","messageId":"xmqqed1a8uhu.fsf@gitster.g","threadId":"62462","inReplyTo":"pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-10T17:10:53Z","receivedAt":"2025-01-10T17:10:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Adam Murray via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Adam Murray <ad@canva.com>\n>\n> When TRACE2 analytics is enabled, a git config option that has no value\n> causes a segfault.\n\nWe often call this \"valueless true syntax\".  It may techincally\ncorrect to say \"has no value\", but it is more friendly to readers if\nyou said \"a configuration variable that is set to 'true' with\nthe valueless true syntax\".\n\n> diff --git a/trace2.c b/trace2.c\n> index f894532d053..49e7d1db88f 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n>  \tif (!trace2_enabled)\n>  \t\treturn;\n>  \n> -\tredacted = redact_arg(value);\n> +\tredacted = value ? redact_arg(value): NULL;\n>  \n>  \tfor_each_wanted_builtin (j, tgt_j)\n>  \t\tif (tgt_j->pfn_param_fl)\n> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\n> index 45b0850a5ec..8e09485c83c 100644\n> --- a/trace2/tr2_tgt_event.c\n> +++ b/trace2/tr2_tgt_event.c\n> @@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n>  \tjw_object_string(&jw, \"scope\", scope_name);\n>  \tjw_object_string(&jw, \"param\", param);\n> -\tjw_object_string(&jw, \"value\", value);\n> +\tif (value)\n> +\t\tjw_object_string(&jw, \"value\", value);\n>  \tjw_end(&jw);\n\nOK, so for \"valueless true\", we do not get the \"value\" element in\nthe json output, which makes sense.  Don't we have documentation\nthat explains what each element in the output means and when they\nare given?  Shouldn't we update it?\n\n> -\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n> -\t\t    value);\n> +\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n> +\tif (value)\n> +\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n\nOK.\n\nThe input did not spell the \"=value\" part and said \"[section] key\"\nto mean that section.key is true, so we report that without \"=value\"\npart.  This also makes sense.\n\n> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\n> index a6f9a8a193e..19ae7433ef8 100644\n> --- a/trace2/tr2_tgt_perf.c\n> +++ b/trace2/tr2_tgt_perf.c\n> @@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tstruct strbuf scope_payload = STRBUF_INIT;\n>  \tenum config_scope scope = kvi->scope;\n>  \tconst char *scope_name = config_scope_name(scope);\n> -\n> -\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n> +\tstrbuf_addstr(&buf_payload, param);\n> +\tif (value)\n> +\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n\nI am not versed well enough in tgt-parf output format to tell if\nthis makes sense.  We'd need somebody else to review this part.\n\nThanks.\n"},{"id":"511048","messageId":"c1398676-ddd7-c161-e114-2249316b5311@gmx.de","threadId":"62462","inReplyTo":"pull.1814.v2.git.1736494100622.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-22T10:11:14Z","receivedAt":"2025-01-22T10:11:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Adam,\n\nOn Fri, 10 Jan 2025, Adam Murray via GitGitGadget wrote:\n\n> From: Adam Murray <ad@canva.com>\n>\n> When TRACE2 analytics is enabled, a git config option that has no value\n> causes a segfault.\n>\n> Steps to Reproduce\n> GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*\n> git -c status.relativePaths version\n> Expected Result\n> git version 2.46.0\n> Actual Result\n> zsh: segmentation fault GIT_TRACE2=true\n>\n> This adds checks to prevent the segfault and instead return\n> an empty value.\n>\n> Signed-off-by: Adam Murray <ad@canva.com>\n\nThis patch looks good to me!\n\nThank you for the fix,\nJohannes\n\n> ---\n>     trace2: prevent segfault on config collection where no value specified\n>\n>     cc: Jeff King peff@peff.net\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1814\n>\n> Range-diff vs v1:\n>\n>  1:  24ba9db7aa1 ! 1:  fd7bed52dda trace2: prevent segfault on config collection where no value specified\n>      @@ Commit message\n>           Actual Result\n>           zsh: segmentation fault GIT_TRACE2=true\n>\n>      -    This adds a null check to prevent the segfault and instead return\n>      -    the \"empty config value\" error.\n>      +    This adds checks to prevent the segfault and instead return\n>      +    an empty value.\n>\n>           Signed-off-by: Adam Murray <ad@canva.com>\n>\n>      @@ t/t0210-trace2-normal.sh: test_expect_success 'bug messages followed by BUG() ar\n>\n>        ## trace2.c ##\n>       @@ trace2.c: void trace2_def_param_fl(const char *file, int line, const char *param,\n>      - \tint j;\n>      - \tconst char *redacted;\n>      -\n>      --\tif (!trace2_enabled)\n>      -+\tif (!trace2_enabled || !value)\n>      + \tif (!trace2_enabled)\n>        \t\treturn;\n>\n>      - \tredacted = redact_arg(value);\n>      +-\tredacted = redact_arg(value);\n>      ++\tredacted = value ? redact_arg(value): NULL;\n>      +\n>      + \tfor_each_wanted_builtin (j, tgt_j)\n>      + \t\tif (tgt_j->pfn_param_fl)\n>      +\n>      + ## trace2/tr2_tgt_event.c ##\n>      +@@ trace2/tr2_tgt_event.c: static void fn_param_fl(const char *file, int line, const char *param,\n>      + \tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n>      + \tjw_object_string(&jw, \"scope\", scope_name);\n>      + \tjw_object_string(&jw, \"param\", param);\n>      +-\tjw_object_string(&jw, \"value\", value);\n>      ++\tif (value)\n>      ++\t\tjw_object_string(&jw, \"value\", value);\n>      + \tjw_end(&jw);\n>      +\n>      + \ttr2_dst_write_line(&tr2dst_event, &jw.json);\n>      +\n>      + ## trace2/tr2_tgt_normal.c ##\n>      +@@ trace2/tr2_tgt_normal.c: static void fn_param_fl(const char *file, int line, const char *param,\n>      + \tenum config_scope scope = kvi->scope;\n>      + \tconst char *scope_name = config_scope_name(scope);\n>      +\n>      +-\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n>      +-\t\t    value);\n>      ++\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n>      ++\tif (value)\n>      ++\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n>      + \tnormal_io_write_fl(file, line, &buf_payload);\n>      + \tstrbuf_release(&buf_payload);\n>      + }\n>      +\n>      + ## trace2/tr2_tgt_perf.c ##\n>      +@@ trace2/tr2_tgt_perf.c: static void fn_param_fl(const char *file, int line, const char *param,\n>      + \tstruct strbuf scope_payload = STRBUF_INIT;\n>      + \tenum config_scope scope = kvi->scope;\n>      + \tconst char *scope_name = config_scope_name(scope);\n>      +-\n>      +-\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n>      ++\tstrbuf_addstr(&buf_payload, param);\n>      ++\tif (value)\n>      ++\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n>      + \tstrbuf_addf(&scope_payload, \"%s:%s\", \"scope\", scope_name);\n>      +\n>      + \tperf_io_write_fl(file, line, event_name, NULL, NULL, NULL,\n>\n>\n>  t/t0210-trace2-normal.sh | 8 ++++++++\n>  trace2.c                 | 2 +-\n>  trace2/tr2_tgt_event.c   | 3 ++-\n>  trace2/tr2_tgt_normal.c  | 5 +++--\n>  trace2/tr2_tgt_perf.c    | 5 +++--\n>  5 files changed, 17 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n> index b9adc94aab4..4047ab562a4 100755\n> --- a/t/t0210-trace2-normal.sh\n> +++ b/t/t0210-trace2-normal.sh\n> @@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'empty configuration values are handled' '\n> +  test_when_finished \"rm trace2.normal actual expect\" &&\n> +  echo >expect &&\n> +  GIT_TRACE2=\"$(pwd)/trace2.normal\"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \\\n> +\t  git -c foo.empty config foo.empty >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  sane_unset GIT_TRACE2_BRIEF\n>\n>  # Now test without environment variables and get all Trace2 settings\n> diff --git a/trace2.c b/trace2.c\n> index f894532d053..49e7d1db88f 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,\n>  \tif (!trace2_enabled)\n>  \t\treturn;\n>\n> -\tredacted = redact_arg(value);\n> +\tredacted = value ? redact_arg(value): NULL;\n>\n>  \tfor_each_wanted_builtin (j, tgt_j)\n>  \t\tif (tgt_j->pfn_param_fl)\n> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\n> index 45b0850a5ec..8e09485c83c 100644\n> --- a/trace2/tr2_tgt_event.c\n> +++ b/trace2/tr2_tgt_event.c\n> @@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n>  \tjw_object_string(&jw, \"scope\", scope_name);\n>  \tjw_object_string(&jw, \"param\", param);\n> -\tjw_object_string(&jw, \"value\", value);\n> +\tif (value)\n> +\t\tjw_object_string(&jw, \"value\", value);\n>  \tjw_end(&jw);\n>\n>  \ttr2_dst_write_line(&tr2dst_event, &jw.json);\n> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\n> index baef48aa698..924736ab360 100644\n> --- a/trace2/tr2_tgt_normal.c\n> +++ b/trace2/tr2_tgt_normal.c\n> @@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tenum config_scope scope = kvi->scope;\n>  \tconst char *scope_name = config_scope_name(scope);\n>\n> -\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s=%s\", scope_name, param,\n> -\t\t    value);\n> +\tstrbuf_addf(&buf_payload, \"def_param scope:%s %s\", scope_name, param);\n> +\tif (value)\n> +\t\tstrbuf_addf(&buf_payload, \"=%s\", value);\n>  \tnormal_io_write_fl(file, line, &buf_payload);\n>  \tstrbuf_release(&buf_payload);\n>  }\n> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\n> index a6f9a8a193e..19ae7433ef8 100644\n> --- a/trace2/tr2_tgt_perf.c\n> +++ b/trace2/tr2_tgt_perf.c\n> @@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>  \tstruct strbuf scope_payload = STRBUF_INIT;\n>  \tenum config_scope scope = kvi->scope;\n>  \tconst char *scope_name = config_scope_name(scope);\n> -\n> -\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n> +\tstrbuf_addstr(&buf_payload, param);\n> +\tif (value)\n> +\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n>  \tstrbuf_addf(&scope_payload, \"%s:%s\", \"scope\", scope_name);\n>\n>  \tperf_io_write_fl(file, line, event_name, NULL, NULL, NULL,\n>\n> base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n> --\n> gitgitgadget\n>\n>\n"},{"id":"511082","messageId":"xmqqfrlawwd9.fsf@gitster.g","threadId":"62462","inReplyTo":"c1398676-ddd7-c161-e114-2249316b5311@gmx.de","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-22T18:12:50Z","receivedAt":"2025-01-22T18:12:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>\n> This patch looks good to me!\n\nThanks.  As I punted on reviewing the tgt_perf part, it is very good\nto see somebody else step in to look it over.\n\nWill queue.\n\n>> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\n>> index a6f9a8a193e..19ae7433ef8 100644\n>> --- a/trace2/tr2_tgt_perf.c\n>> +++ b/trace2/tr2_tgt_perf.c\n>> @@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,\n>>  \tstruct strbuf scope_payload = STRBUF_INIT;\n>>  \tenum config_scope scope = kvi->scope;\n>>  \tconst char *scope_name = config_scope_name(scope);\n>> -\n>> -\tstrbuf_addf(&buf_payload, \"%s:%s\", param, value);\n>> +\tstrbuf_addstr(&buf_payload, param);\n>> +\tif (value)\n>> +\t\tstrbuf_addf(&buf_payload, \":%s\", value);\n>>  \tstrbuf_addf(&scope_payload, \"%s:%s\", \"scope\", scope_name);\n>>\n>>  \tperf_io_write_fl(file, line, event_name, NULL, NULL, NULL,\n>>\n>> base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n>> --\n>> gitgitgadget\n>>\n>>\n"},{"id":"511083","messageId":"xmqqbjvyww3j.fsf@gitster.g","threadId":"62462","inReplyTo":"xmqqfrlawwd9.fsf@gitster.g","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-22T18:18:40Z","receivedAt":"2025-01-22T18:18:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>>\n>> This patch looks good to me!\n>\n> Thanks.  As I punted on reviewing the tgt_perf part, it is very good\n> to see somebody else step in to look it over.\n>\n> Will queue.\n\nThe test part used broken indentation and also the use of\ntest_when_finished was careless, so I'll touch it up before queuing.\n\nNo need to resend.\n\nThanks.\n"},{"id":"511111","messageId":"CALnO6CDH1aQrq0tcvr4rEGukksYv5bJ9W4icqBUC2Z3AyixRUw@mail.gmail.com","threadId":"62462","inReplyTo":"xmqqbjvyww3j.fsf@gitster.g","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-01-23T17:01:40Z","receivedAt":"2025-01-23T17:01:55Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, Jan 22, 2025 at 1:18 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> >\n> >>\n> >> This patch looks good to me!\n> >\n> > Thanks.  As I punted on reviewing the tgt_perf part, it is very good\n> > to see somebody else step in to look it over.\n> >\n> > Will queue.\n>\n> The test part used broken indentation and also the use of\n> test_when_finished was careless, so I'll touch it up before queuing.\n>\n> No need to resend.\n>\n> Thanks.\n>\n\nI was curious what changed, so I found 792a3850fa (trace2: prevent\nsegfault on config collection with valueless true, 2025-01-10) and I\nnoticed this:\n\n-       redacted = redact_arg(value);\n+       redacted = value ? redact_arg(value): NULL;\n\nI think I expected (based on Documentation/CodingGuidelines) a space\nbefore the ternary conditional's colon. Then again, \"git grep '?.*[^\n]:' *.[ch]\" finds a few other cases with the unspaced style.\n\n-- \nD. Ben Knoble\n"},{"id":"511126","messageId":"xmqqr04ts916.fsf@gitster.g","threadId":"62462","inReplyTo":"CALnO6CDH1aQrq0tcvr4rEGukksYv5bJ9W4icqBUC2Z3AyixRUw@mail.gmail.com","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-23T18:02:45Z","receivedAt":"2025-01-23T18:02:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" <ben.knoble@gmail.com> writes:\n\n> I was curious what changed, so I found 792a3850fa (trace2: prevent\n> segfault on config collection with valueless true, 2025-01-10) and I\n> noticed this:\n>\n> -       redacted = redact_arg(value);\n> +       redacted = value ? redact_arg(value): NULL;\n\nOh, I do not think I tweaked any of the code, other than fixing the\nnew test that was not properly formatted.  Mostly I touched up the\nlog message.\n\nWill touch it up.  Thanks for a careful reading.\n"},{"id":"511133","messageId":"CALnO6CAcv+544jRdp9VO1d38dvHX6MNfu4VLKenjKZCsCaf14Q@mail.gmail.com","threadId":"62462","inReplyTo":"xmqqr04ts916.fsf@gitster.g","subject":"Re: [PATCH v2] trace2: prevent segfault on config collection where no value specified","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-01-23T21:15:12Z","receivedAt":"2025-01-23T21:15:25Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Thu, Jan 23, 2025 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"D. Ben Knoble\" <ben.knoble@gmail.com> writes:\n>\n> > I was curious what changed, so I found 792a3850fa (trace2: prevent\n> > segfault on config collection with valueless true, 2025-01-10) and I\n> > noticed this:\n> >\n> > -       redacted = redact_arg(value);\n> > +       redacted = value ? redact_arg(value): NULL;\n>\n> Oh, I do not think I tweaked any of the code, other than fixing the\n> new test that was not properly formatted.  Mostly I touched up the\n> log message.\n\nI could have been clearer—I think this was in the original. I just\nhappened to notice it because I was curious about what touch-ups you\nperformed :)\n\n\n\n-- \nD. Ben Knoble\n"}]}