{"thread":{"id":"60593","subject":"[PATCH 0/7] fix segfaults with implicit-bool config","startedAt":"2023-12-07T07:10:31Z","lastAt":"2023-12-12T04:10:05Z","messageCount":17,"participants":["Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"485416","messageId":"20231207071030.GA1275835@coredump.intra.peff.net","threadId":"60593","inReplyTo":null,"subject":"[PATCH 0/7] fix segfaults with implicit-bool config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:10:30Z","receivedAt":"2023-12-07T07:10:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Carlos reported to the security list a case where you can cause Git\nto segfault by using an implicit bool like:\n\n  [core]\n  someVariable\n\nwhen the parsing side for core.someVariable does not correctly check a\nNULL \"value\" string. This is mostly harmless, as anybody who can feed\narbitrary config can already execute arbitrary code. There is one case\nof this when parsing .gitmodules (which we don't trust), but even there\nI don't think the security implications are that interesting. A\nmalicious repo can get \"clone --recurse-submodules\" to segfault, but\nalways with a strict NULL dereference, not any kind of controllable\npointer. See patch 5 for more details.\n\nI audited the whole code base for instances of the problem. It was\nfairly manual, so it's possible I missed a spot, but I think this should\ncover everything.\n\nThe first patch has vanilla cases, and the rest are instances where I\nthought it was worth calling out specific details.\n\n  [1/7]: config: handle NULL value when parsing non-bools\n  [2/7]: setup: handle NULL value when parsing extensions\n  [3/7]: trace2: handle NULL values in tr2_sysenv config callback\n  [4/7]: help: handle NULL value for alias.* config\n  [5/7]: submodule: handle NULL value when parsing submodule.*.branch\n  [6/7]: trailer: handle NULL value when parsing trailer-specific config\n  [7/7]: fsck: handle NULL value when parsing message config\n\n builtin/blame.c        |  2 ++\n builtin/checkout.c     |  2 ++\n builtin/clone.c        |  2 ++\n builtin/log.c          |  5 ++++-\n builtin/pack-objects.c |  6 +++++-\n builtin/receive-pack.c | 11 +++++++----\n compat/mingw.c         |  2 ++\n config.c               |  8 ++++++++\n diff.c                 | 19 ++++++++++++++++---\n fetch-pack.c           | 12 ++++++++----\n fsck.c                 |  8 ++++++--\n help.c                 |  5 ++++-\n mailinfo.c             |  2 ++\n notes-utils.c          |  2 ++\n setup.c                |  2 ++\n submodule-config.c     |  4 +++-\n trace2/tr2_sysenv.c    |  2 ++\n trailer.c              |  8 ++++++++\n 18 files changed, 85 insertions(+), 17 deletions(-)\n"},{"id":"485417","messageId":"20231207071114.GA1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 1/7] config: handle NULL value when parsing non-bools","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:14Z","receivedAt":"2023-12-07T07:11:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the config parser sees an \"implicit\" bool like:\n\n  [core]\n  someVariable\n\nit passes NULL to the config callback. Any callback code which expects a\nstring must check for NULL. This usually happens via helpers like\ngit_config_string(), etc, but some custom code forgets to do so and will\nsegfault.\n\nThese are all fairly vanilla cases where the solution is just the usual\npattern of:\n\n  if (!value)\n        return config_error_nonbool(var);\n\nthough note that in a few cases we have to split initializers like:\n\n  int some_var = initializer();\n\ninto:\n\n  int some_var;\n  if (!value)\n        return config_error_nonbool(var);\n  some_var = initializer();\n\nThere are still some broken instances after this patch, which I'll\naddress on their own in individual patches after this one.\n\nReported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c        |  2 ++\n builtin/checkout.c     |  2 ++\n builtin/clone.c        |  2 ++\n builtin/log.c          |  5 ++++-\n builtin/pack-objects.c |  6 +++++-\n compat/mingw.c         |  2 ++\n config.c               |  8 ++++++++\n diff.c                 | 19 ++++++++++++++++---\n mailinfo.c             |  2 ++\n notes-utils.c          |  2 ++\n trailer.c              |  2 ++\n 11 files changed, 47 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9c987d6567..2433b7da5c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"blame.coloring\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (!strcmp(value, \"repeatedLines\")) {\n \t\t\tcoloring_mode |= OUTPUT_COLOR_LINE;\n \t\t} else if (!strcmp(value, \"highlightRecent\")) {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f02434bc15..d5c784854f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,\n \tstruct checkout_opts *opts = cb;\n \n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\thandle_ignore_submodules_arg(&opts->diff_options, value);\n \t\treturn 0;\n \t}\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af949..54d9b9976a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -791,6 +791,8 @@ static int git_clone_config(const char *k, const char *v,\n \t\t\t    const struct config_context *ctx, void *cb)\n {\n \tif (!strcmp(k, \"clone.defaultremotename\")) {\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n \t\tfree(remote_name);\n \t\tremote_name = xstrdup(v);\n \t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ba775d7b5c..3ce41c4856 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -594,8 +594,11 @@ static int git_log_config(const char *var, const char *value,\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"log.diffmerges\"))\n+\tif (!strcmp(var, \"log.diffmerges\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\treturn diff_merges_config(value);\n+\t}\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 89a8b5a976..62c540b4db 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n-\t\tstruct configured_exclusion *ex = xmalloc(sizeof(*ex));\n+\t\tstruct configured_exclusion *ex;\n \t\tconst char *oid_end, *pack_end;\n \t\t/*\n \t\t * Stores the pack hash. This is not a true object ID, but is\n \t\t * of the same form.\n \t\t */\n \t\tstruct object_id pack_hash;\n \n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n+\n+\t\tex = xmalloc(sizeof(*ex));\n \t\tif (parse_oid_hex(v, &ex->e.oid, &oid_end) ||\n \t\t    *oid_end != ' ' ||\n \t\t    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex ec5280da16..42053c1f65 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -255,6 +255,8 @@ int mingw_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.unsetenvvars\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tfree(unset_environment_variables);\n \t\tunset_environment_variables = xstrdup(value);\n \t\treturn 0;\ndiff --git a/config.c b/config.c\nindex b330c7adb4..18085c7e38 100644\n--- a/config.c\n+++ b/config.c\n@@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"core.checkstat\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (!strcasecmp(value, \"default\"))\n \t\t\tcheck_stat = 1;\n \t\telse if (!strcasecmp(value, \"minimal\"))\n@@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.checkroundtripencoding\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tcheck_roundtrip_encoding = xstrdup(value);\n \t\treturn 0;\n \t}\n \n \tif (!strcmp(var, \"core.notesref\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tnotes_ref_name = xstrdup(value);\n \t\treturn 0;\n \t}\n@@ -1619,6 +1625,8 @@ static int git_default_core_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"core.createobject\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (!strcmp(value, \"rename\"))\n \t\t\tobject_creation_mode = OBJECT_CREATION_USES_RENAMES;\n \t\telse if (!strcmp(value, \"link\"))\ndiff --git a/diff.c b/diff.c\nindex 2c602df10a..5b213a4b44 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -372,7 +372,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"diff.colormovedws\")) {\n-\t\tunsigned cm = parse_color_moved_ws(value);\n+\t\tunsigned cm;\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tcm = parse_color_moved_ws(value);\n \t\tif (cm & COLOR_MOVED_WS_ERROR)\n \t\t\treturn -1;\n \t\tdiff_color_moved_ws_default = cm;\n@@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,\n \tif (!strcmp(var, \"diff.orderfile\"))\n \t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n \n-\tif (!strcmp(var, \"diff.ignoresubmodules\"))\n+\tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n+\t}\n \n \tif (!strcmp(var, \"diff.submodule\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (parse_submodule_params(&default_diff_options, value))\n \t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: '%s'\"),\n \t\t\t\tvalue);\n@@ -473,7 +481,10 @@ int git_diff_basic_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n-\t\tint val = parse_ws_error_highlight(value);\n+\t\tint val;\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tval = parse_ws_error_highlight(value);\n \t\tif (val < 0)\n \t\t\treturn -1;\n \t\tws_error_highlight_default = val;\n@@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,\n \n \tif (!strcmp(var, \"diff.dirstat\")) {\n \t\tstruct strbuf errmsg = STRBUF_INIT;\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tdefault_diff_options.dirstat_permille = diff_dirstat_permille_default;\n \t\tif (parse_dirstat_params(&default_diff_options, value, &errmsg))\n \t\t\twarning(_(\"Found errors in 'diff.dirstat' config variable:\\n%s\"),\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a07d2da16d..093bed5d8f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1253,6 +1253,8 @@ static int git_mailinfo_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"mailinfo.quotedcr\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (mailinfo_parse_quoted_cr_action(value, &mi->quoted_cr) != 0)\n \t\t\treturn error(_(\"bad action '%s' for '%s'\"), value, var);\n \t\treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex 97c031c26e..01f4f5b424 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,\n \t\t}\n \t\treturn 0;\n \t} else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n \t\t/* note that a refs/ prefix is implied in the\n \t\t * underlying for_each_glob_ref */\n \t\tif (starts_with(v, \"refs/notes/\"))\ndiff --git a/trailer.c b/trailer.c\nindex b6de5d9cb2..b0e2ec224a 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,\n \t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"),\n \t\t\t\t\tvalue, conf_key);\n \t\t} else if (!strcmp(trailer_item, \"separators\")) {\n+\t\t\tif (!value)\n+\t\t\t\treturn config_error_nonbool(conf_key);\n \t\t\tseparators = xstrdup(value);\n \t\t}\n \t}\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485419","messageId":"20231207071121.GB1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 2/7] setup: handle NULL value when parsing extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:21Z","receivedAt":"2023-12-07T07:11:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"partialclone\" extension config records a string, and hence it is an\nerror to have an implicit bool like:\n\n  [extensions]\n  partialclone\n\nin your config. We should recognize and reject this, rather than\nsegfaulting (which is the current behavior). Note that it's OK to use\nconfig_error_nonbool() here, even though the return value is an enum. We\nexplicitly document EXTENSION_ERROR as -1 for compatibility with\nerror(), etc.\n\nThis is the only extension value that has this problem. Most of the\nothers are bools that interpret this value naturally. The exception is\nextensions.objectformat, which does correctly check for NULL.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n setup.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..50131be7cc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -559,6 +559,8 @@ static enum extension_result handle_extension_v0(const char *var,\n \t\t\tdata->precious_objects = git_config_bool(var, value);\n \t\t\treturn EXTENSION_OK;\n \t\t} else if (!strcmp(ext, \"partialclone\")) {\n+\t\t\tif (!value)\n+\t\t\t\treturn config_error_nonbool(var);\n \t\t\tdata->partial_clone = xstrdup(value);\n \t\t\treturn EXTENSION_OK;\n \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485418","messageId":"20231207071124.GC1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 3/7] trace2: handle NULL values in tr2_sysenv config callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:24Z","receivedAt":"2023-12-07T07:11:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you have config with an implicit bool like:\n\n  [trace2]\n  envvars\n\nwe'll segfault, as we unconditionally try to xstrdup() the value. We\nshould instead detect and complain, as a boolean value has no meaning\nhere. The same is true for every variable in tr2_sysenv_settings (and\nthis patch covers them all, as we check them in a loop).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n trace2/tr2_sysenv.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex d3ecac2772..048cdd5438 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -68,6 +68,8 @@ static int tr2_sysenv_cb(const char *key, const char *value,\n \n \tfor (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++) {\n \t\tif (!strcmp(key, tr2_sysenv_settings[k].git_config_name)) {\n+\t\t\tif (!value)\n+\t\t\t\treturn config_error_nonbool(key);\n \t\t\tfree(tr2_sysenv_settings[k].value);\n \t\t\ttr2_sysenv_settings[k].value = xstrdup(value);\n \t\t\treturn 0;\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485420","messageId":"20231207071127.GD1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 4/7] help: handle NULL value for alias.* config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:27Z","receivedAt":"2023-12-07T07:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When showing all config with \"git help --all\", we print the list of\ndefined aliases. But our config callback to do so does not check for a\nNULL value, meaning a config block like:\n\n  [alias]\n  foo\n\nwill cause us to segfault. We should detect and complain about this in\nthe usual way.\n\nSince this command is purely informational (and we aren't trying to run\nthe alias), we could perhaps just generate a warning and continue. But\nthis sort of misconfiguration should be pretty rare, and the error\nmessage we will produce points directly to the line of config that needs\nto be fixed. So just generating the usual error should be OK.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n help.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/help.c b/help.c\nindex 6d2ebfbd2a..2dbe57b413 100644\n--- a/help.c\n+++ b/help.c\n@@ -464,8 +464,11 @@ static int get_alias(const char *var, const char *value,\n {\n \tstruct string_list *list = data;\n \n-\tif (skip_prefix(var, \"alias.\", &var))\n+\tif (skip_prefix(var, \"alias.\", &var)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tstring_list_append(list, var)->util = xstrdup(value);\n+\t}\n \n \treturn 0;\n }\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485421","messageId":"20231207071129.GE1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:29Z","receivedAt":"2023-12-07T07:11:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We record the submodule branch config value as a string, so config that\nuses an implicit bool like:\n\n  [submodule \"foo\"]\n  branch\n\nwill cause us to segfault. Note that unlike most other config-parsing\nbugs of this class, this can be triggered by parsing a bogus .gitmodules\nfile (which we might do after cloning a malicious repository).\n\nI don't think the security implications are important, though. It's\nalways a strict NULL dereference, not an out-of-bounds read or write. So\nwe should reliably kill the process. That may be annoying, but the\nimpact is limited to the attacker preventing the victim from\nsuccessfully using \"git clone --recurse-submodules\", etc, on the\nmalicious repo.\n\nThe \"branch\" entry is the only one with this problem; other strings like\n\"path\" and \"url\" already check for NULL.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n submodule-config.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 6a48fd12f6..f4dd482abc 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,\n \t\t\tsubmodule->recommend_shallow =\n \t\t\t\tgit_config_bool(var, value);\n \t} else if (!strcmp(item.buf, \"branch\")) {\n-\t\tif (!me->overwrite && submodule->branch)\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->branch)\n \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n \t\t\t\t\t     \"branch\");\n \t\telse {\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485422","messageId":"20231207071132.GF1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:32Z","receivedAt":"2023-12-07T07:11:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing the \"key\", \"command\", and \"cmd\" trailer config, we just\nmake a copy of the value string.  If we see an implicit bool like:\n\n  [trailer \"foo\"]\n  key\n\nwe'll segfault trying to copy a NULL pointer. We can fix this with the\nusual config_error_nonbool() check.\n\nI split this out from the other vanilla cases, because at first glance\nit looks like a better fix here would be to move the NULL check out of\nthe switch statement. But it would change the behavior of other keys\nlike trailer.*.ifExists, where an implicit bool is interpreted as\nEXISTS_DEFAULT.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n trailer.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex b0e2ec224a..e4b08ed267 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value,\n \tcase TRAILER_KEY:\n \t\tif (conf->key)\n \t\t\twarning(_(\"more than one %s\"), conf_key);\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(conf_key);\n \t\tconf->key = xstrdup(value);\n \t\tbreak;\n \tcase TRAILER_COMMAND:\n \t\tif (conf->command)\n \t\t\twarning(_(\"more than one %s\"), conf_key);\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(conf_key);\n \t\tconf->command = xstrdup(value);\n \t\tbreak;\n \tcase TRAILER_CMD:\n \t\tif (conf->cmd)\n \t\t\twarning(_(\"more than one %s\"), conf_key);\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(conf_key);\n \t\tconf->cmd = xstrdup(value);\n \t\tbreak;\n \tcase TRAILER_WHERE:\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485423","messageId":"20231207071135.GG1276005@coredump.intra.peff.net","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"[PATCH 7/7] fsck: handle NULL value when parsing message config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:11:35Z","receivedAt":"2023-12-07T07:11:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for\nan implicit bool. So any of:\n\n  [fsck]\n  badTree\n  [receive \"fsck\"]\n  badTree\n  [fetch \"fsck\"]\n  badTree\n\nwill cause us to segfault. We can fix it with config_error_nonbool() in\nthe usual way, but we have to make a few more changes to get good error\nmessages. The problem is that all three spots do:\n\n  if (skip_prefix(var, \"fsck.\", &var))\n\nto match and parse the actual message id. But that means that \"var\" now\njust says \"badTree\" instead of \"receive.fsck.badTree\", making the\nresulting message confusing. We can fix that by storing the parsed\nmessage id in its own separate variable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 11 +++++++----\n fetch-pack.c           | 12 ++++++++----\n fsck.c                 |  8 ++++++--\n 3 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 8c4f0cb90a..ccf9738bce 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n static int receive_pack_config(const char *var, const char *value,\n \t\t\t       const struct config_context *ctx, void *cb)\n {\n+\tconst char *msg_id;\n \tint status = parse_hide_refs_config(var, value, \"receive\", &hidden_refs);\n \n \tif (status)\n@@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n-\tif (skip_prefix(var, \"receive.fsck.\", &var)) {\n-\t\tif (is_valid_msg_type(var, value))\n+\tif (skip_prefix(var, \"receive.fsck.\", &msg_id)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif (is_valid_msg_type(msg_id, value))\n \t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n-\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n \t\telse\n-\t\t\twarning(\"skipping unknown msg id '%s'\", var);\n+\t\t\twarning(\"skipping unknown msg id '%s'\", msg_id);\n \t\treturn 0;\n \t}\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 26999e3b65..31a72d43de 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1862,6 +1862,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n static int fetch_pack_config_cb(const char *var, const char *value,\n \t\t\t\tconst struct config_context *ctx, void *cb)\n {\n+\tconst char *msg_id;\n+\n \tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n \t\tconst char *path;\n \n@@ -1873,12 +1875,14 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n-\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n-\t\tif (is_valid_msg_type(var, value))\n+\tif (skip_prefix(var, \"fetch.fsck.\", &msg_id)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif (is_valid_msg_type(msg_id, value))\n \t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n-\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n \t\telse\n-\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n+\t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n \t\treturn 0;\n \t}\n \ndiff --git a/fsck.c b/fsck.c\nindex 6a0bbc5087..b624083a13 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1403,6 +1403,8 @@ int git_fsck_config(const char *var, const char *value,\n \t\t    const struct config_context *ctx, void *cb)\n {\n \tstruct fsck_options *options = cb;\n+\tconst char *msg_id;\n+\n \tif (strcmp(var, \"fsck.skiplist\") == 0) {\n \t\tconst char *path;\n \t\tstruct strbuf sb = STRBUF_INIT;\n@@ -1416,8 +1418,10 @@ int git_fsck_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n-\tif (skip_prefix(var, \"fsck.\", &var)) {\n-\t\tfsck_set_msg_type(options, var, value);\n+\tif (skip_prefix(var, \"fsck.\", &msg_id)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tfsck_set_msg_type(options, msg_id, value);\n \t\treturn 0;\n \t}\n \n-- \n2.43.0.664.ga12c899002\n"},{"id":"485437","messageId":"ZXF-7AMZ_SBltplk@tanuki","threadId":"60593","inReplyTo":"20231207071030.GA1275835@coredump.intra.peff.net","subject":"Re: [PATCH 0/7] fix segfaults with implicit-bool config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:14:36Z","receivedAt":"2023-12-07T08:14:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:10:30AM -0500, Jeff King wrote:\n> Carlos reported to the security list a case where you can cause Git\n> to segfault by using an implicit bool like:\n> \n>   [core]\n>   someVariable\n> \n> when the parsing side for core.someVariable does not correctly check a\n> NULL \"value\" string. This is mostly harmless, as anybody who can feed\n> arbitrary config can already execute arbitrary code. There is one case\n> of this when parsing .gitmodules (which we don't trust), but even there\n> I don't think the security implications are that interesting. A\n> malicious repo can get \"clone --recurse-submodules\" to segfault, but\n> always with a strict NULL dereference, not any kind of controllable\n> pointer. See patch 5 for more details.\n> \n> I audited the whole code base for instances of the problem. It was\n> fairly manual, so it's possible I missed a spot, but I think this should\n> cover everything.\n> \n> The first patch has vanilla cases, and the rest are instances where I\n> thought it was worth calling out specific details.\n\nThanks for working on this topic! I've left a couple of comments, most\nof which are about whether we should retain previous behaviour where we\ngenerate a warning instead of raising an error for unknown values.\n\nPatrick\n\n>   [1/7]: config: handle NULL value when parsing non-bools\n>   [2/7]: setup: handle NULL value when parsing extensions\n>   [3/7]: trace2: handle NULL values in tr2_sysenv config callback\n>   [4/7]: help: handle NULL value for alias.* config\n>   [5/7]: submodule: handle NULL value when parsing submodule.*.branch\n>   [6/7]: trailer: handle NULL value when parsing trailer-specific config\n>   [7/7]: fsck: handle NULL value when parsing message config\n> \n>  builtin/blame.c        |  2 ++\n>  builtin/checkout.c     |  2 ++\n>  builtin/clone.c        |  2 ++\n>  builtin/log.c          |  5 ++++-\n>  builtin/pack-objects.c |  6 +++++-\n>  builtin/receive-pack.c | 11 +++++++----\n>  compat/mingw.c         |  2 ++\n>  config.c               |  8 ++++++++\n>  diff.c                 | 19 ++++++++++++++++---\n>  fetch-pack.c           | 12 ++++++++----\n>  fsck.c                 |  8 ++++++--\n>  help.c                 |  5 ++++-\n>  mailinfo.c             |  2 ++\n>  notes-utils.c          |  2 ++\n>  setup.c                |  2 ++\n>  submodule-config.c     |  4 +++-\n>  trace2/tr2_sysenv.c    |  2 ++\n>  trailer.c              |  8 ++++++++\n>  18 files changed, 85 insertions(+), 17 deletions(-)\n> \n"},{"id":"485438","messageId":"ZXF-8iNH0qaJSVl9@tanuki","threadId":"60593","inReplyTo":"20231207071114.GA1276005@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] config: handle NULL value when parsing non-bools","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:14:42Z","receivedAt":"2023-12-07T08:14:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote:\n> When the config parser sees an \"implicit\" bool like:\n> \n>   [core]\n>   someVariable\n> \n> it passes NULL to the config callback. Any callback code which expects a\n> string must check for NULL. This usually happens via helpers like\n> git_config_string(), etc, but some custom code forgets to do so and will\n> segfault.\n> \n> These are all fairly vanilla cases where the solution is just the usual\n> pattern of:\n> \n>   if (!value)\n>         return config_error_nonbool(var);\n> \n> though note that in a few cases we have to split initializers like:\n> \n>   int some_var = initializer();\n> \n> into:\n> \n>   int some_var;\n>   if (!value)\n>         return config_error_nonbool(var);\n>   some_var = initializer();\n> \n> There are still some broken instances after this patch, which I'll\n> address on their own in individual patches after this one.\n> \n> Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/blame.c        |  2 ++\n>  builtin/checkout.c     |  2 ++\n>  builtin/clone.c        |  2 ++\n>  builtin/log.c          |  5 ++++-\n>  builtin/pack-objects.c |  6 +++++-\n>  compat/mingw.c         |  2 ++\n>  config.c               |  8 ++++++++\n>  diff.c                 | 19 ++++++++++++++++---\n>  mailinfo.c             |  2 ++\n>  notes-utils.c          |  2 ++\n>  trailer.c              |  2 ++\n>  11 files changed, 47 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 9c987d6567..2433b7da5c 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,\n>  \t}\n>  \n>  \tif (!strcmp(var, \"blame.coloring\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n\nIn the `else` statement where we fail to parse the value we only\ngenerate a warning and return successfully regardless. Should we do the\nsame here?\n\n>  \t\tif (!strcmp(value, \"repeatedLines\")) {\n>  \t\t\tcoloring_mode |= OUTPUT_COLOR_LINE;\n>  \t\t} else if (!strcmp(value, \"highlightRecent\")) {\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index f02434bc15..d5c784854f 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,\n>  \tstruct checkout_opts *opts = cb;\n>  \n>  \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\thandle_ignore_submodules_arg(&opts->diff_options, value);\n>  \t\treturn 0;\n>  \t}\n\nThis one is fine, `handle_ignore_submodules_arg()` dies if it gets an\nunknown value and `git_config()` will die when the callback function\nreturns an error. The same is true for many other cases you've\nconverted.\n\n[snip]\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 89a8b5a976..62c540b4db 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,\n>  \t\treturn 0;\n>  \t}\n>  \tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n> -\t\tstruct configured_exclusion *ex = xmalloc(sizeof(*ex));\n> +\t\tstruct configured_exclusion *ex;\n>  \t\tconst char *oid_end, *pack_end;\n>  \t\t/*\n>  \t\t * Stores the pack hash. This is not a true object ID, but is\n>  \t\t * of the same form.\n>  \t\t */\n>  \t\tstruct object_id pack_hash;\n>  \n> +\t\tif (!v)\n> +\t\t\treturn config_error_nonbool(k);\n> +\n> +\t\tex = xmalloc(sizeof(*ex));\n>  \t\tif (parse_oid_hex(v, &ex->e.oid, &oid_end) ||\n>  \t\t    *oid_end != ' ' ||\n>  \t\t    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||\n\nThis isn't part of the diff and not a new issue, but why don't we\n`return 0` when parsing this config correctly? We fall through to\n`git_default_config()` even if we've successfully parsed the config key,\nwhich seems like a bug to me.\n\nAnyway, this case looks fine.\n\n[snip]\n> diff --git a/config.c b/config.c\n> index b330c7adb4..18085c7e38 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t\treturn 0;\n>  \t}\n>  \tif (!strcmp(var, \"core.checkstat\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\tif (!strcasecmp(value, \"default\"))\n>  \t\t\tcheck_stat = 1;\n>  \t\telse if (!strcasecmp(value, \"minimal\"))\n\nWe would ignore `true` here, so should we ignore implicit `true`, as\nwell?\n\n> @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,\n>  \t}\n>  \n>  \tif (!strcmp(var, \"core.checkroundtripencoding\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\tcheck_roundtrip_encoding = xstrdup(value);\n>  \t\treturn 0;\n>  \t}\n>  \n>  \tif (!strcmp(var, \"core.notesref\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\tnotes_ref_name = xstrdup(value);\n>  \t\treturn 0;\n>  \t}\n\nI wonder the same here. We might as well use `xstrdup_or_null()`, but it\nfeels like the right thing to do to convert these to actual errors.\n\n> @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,\n>  \tif (!strcmp(var, \"diff.orderfile\"))\n>  \t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n>  \n> -\tif (!strcmp(var, \"diff.ignoresubmodules\"))\n> +\tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n> +\t}\n>  \n>  \tif (!strcmp(var, \"diff.submodule\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\tif (parse_submodule_params(&default_diff_options, value))\n>  \t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: '%s'\"),\n>  \t\t\t\tvalue);\n\nShould we generate a warning instead according to the preexisting code\nfor \"diff.submodule\"?\n\n> @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,\n>  \n>  \tif (!strcmp(var, \"diff.dirstat\")) {\n>  \t\tstruct strbuf errmsg = STRBUF_INIT;\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n>  \t\tdefault_diff_options.dirstat_permille = diff_dirstat_permille_default;\n>  \t\tif (parse_dirstat_params(&default_diff_options, value, &errmsg))\n>  \t\t\twarning(_(\"Found errors in 'diff.dirstat' config variable:\\n%s\"),\n\nSame here, should we generate a warning instead?\n\n> diff --git a/notes-utils.c b/notes-utils.c\n> index 97c031c26e..01f4f5b424 100644\n> --- a/notes-utils.c\n> +++ b/notes-utils.c\n> @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,\n>  \t\t}\n>  \t\treturn 0;\n>  \t} else if (!c->refs_from_env && !strcmp(k, \"notes.rewriteref\")) {\n> +\t\tif (!v)\n> +\t\t\treturn config_error_nonbool(k);\n>  \t\t/* note that a refs/ prefix is implied in the\n>  \t\t * underlying for_each_glob_ref */\n>  \t\tif (starts_with(v, \"refs/notes/\"))\n\nHere, as well.\n\n> diff --git a/trailer.c b/trailer.c\n> index b6de5d9cb2..b0e2ec224a 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,\n>  \t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"),\n>  \t\t\t\t\tvalue, conf_key);\n>  \t\t} else if (!strcmp(trailer_item, \"separators\")) {\n> +\t\t\tif (!value)\n> +\t\t\t\treturn config_error_nonbool(conf_key);\n>  \t\t\tseparators = xstrdup(value);\n>  \t\t}\n>  \t}\n\nAnd here.\n\nPatrick\n"},{"id":"485439","messageId":"ZXF--AxCOOOjyOMc@tanuki","threadId":"60593","inReplyTo":"20231207071129.GE1276005@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:14:48Z","receivedAt":"2023-12-07T08:14:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:11:29AM -0500, Jeff King wrote:\n> We record the submodule branch config value as a string, so config that\n> uses an implicit bool like:\n> \n>   [submodule \"foo\"]\n>   branch\n> \n> will cause us to segfault. Note that unlike most other config-parsing\n> bugs of this class, this can be triggered by parsing a bogus .gitmodules\n> file (which we might do after cloning a malicious repository).\n> \n> I don't think the security implications are important, though. It's\n> always a strict NULL dereference, not an out-of-bounds read or write. So\n> we should reliably kill the process. That may be annoying, but the\n> impact is limited to the attacker preventing the victim from\n> successfully using \"git clone --recurse-submodules\", etc, on the\n> malicious repo.\n> \n> The \"branch\" entry is the only one with this problem; other strings like\n> \"path\" and \"url\" already check for NULL.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  submodule-config.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/submodule-config.c b/submodule-config.c\n> index 6a48fd12f6..f4dd482abc 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,\n>  \t\t\tsubmodule->recommend_shallow =\n>  \t\t\t\tgit_config_bool(var, value);\n>  \t} else if (!strcmp(item.buf, \"branch\")) {\n> -\t\tif (!me->overwrite && submodule->branch)\n> +\t\tif (!value)\n> +\t\t\tret = config_error_nonbool(var);\n> +\t\telse if (!me->overwrite && submodule->branch)\n>  \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n>  \t\t\t\t\t     \"branch\");\n>  \t\telse {\n\nI was about to say that I'd rather expect us to handle this gracefully\nso that Git doesn't die when parsing an invalid gitmodules file. But\nthere are other cases where we already fail in the same way, so this\nlooks good to me.\n\nPatrick\n"},{"id":"485440","messageId":"ZXF-_bS6upwHIWMh@tanuki","threadId":"60593","inReplyTo":"20231207071132.GF1276005@coredump.intra.peff.net","subject":"Re: [PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:14:53Z","receivedAt":"2023-12-07T08:14:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:11:32AM -0500, Jeff King wrote:\n> When parsing the \"key\", \"command\", and \"cmd\" trailer config, we just\n> make a copy of the value string.  If we see an implicit bool like:\n> \n>   [trailer \"foo\"]\n>   key\n> \n> we'll segfault trying to copy a NULL pointer. We can fix this with the\n> usual config_error_nonbool() check.\n> \n> I split this out from the other vanilla cases, because at first glance\n> it looks like a better fix here would be to move the NULL check out of\n> the switch statement. But it would change the behavior of other keys\n> like trailer.*.ifExists, where an implicit bool is interpreted as\n> EXISTS_DEFAULT.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  trailer.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/trailer.c b/trailer.c\n> index b0e2ec224a..e4b08ed267 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value,\n>  \tcase TRAILER_KEY:\n>  \t\tif (conf->key)\n>  \t\t\twarning(_(\"more than one %s\"), conf_key);\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(conf_key);\n>  \t\tconf->key = xstrdup(value);\n>  \t\tbreak;\n>  \tcase TRAILER_COMMAND:\n>  \t\tif (conf->command)\n>  \t\t\twarning(_(\"more than one %s\"), conf_key);\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(conf_key);\n>  \t\tconf->command = xstrdup(value);\n>  \t\tbreak;\n>  \tcase TRAILER_CMD:\n>  \t\tif (conf->cmd)\n>  \t\t\twarning(_(\"more than one %s\"), conf_key);\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(conf_key);\n>  \t\tconf->cmd = xstrdup(value);\n>  \t\tbreak;\n>  \tcase TRAILER_WHERE:\n\nFor the other cases we only generate warnings for unknown values, but\nreturn successfully. Should we do the same here?\n\nPatrick\n"},{"id":"485441","messageId":"ZXF_BHFubnPJWNhQ@tanuki","threadId":"60593","inReplyTo":"20231207071135.GG1276005@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] fsck: handle NULL value when parsing message config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:15:00Z","receivedAt":"2023-12-07T08:15:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:11:35AM -0500, Jeff King wrote:\n> When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for\n> an implicit bool. So any of:\n> \n>   [fsck]\n>   badTree\n>   [receive \"fsck\"]\n>   badTree\n>   [fetch \"fsck\"]\n>   badTree\n> \n> will cause us to segfault. We can fix it with config_error_nonbool() in\n> the usual way, but we have to make a few more changes to get good error\n> messages. The problem is that all three spots do:\n> \n>   if (skip_prefix(var, \"fsck.\", &var))\n> \n> to match and parse the actual message id. But that means that \"var\" now\n> just says \"badTree\" instead of \"receive.fsck.badTree\", making the\n> resulting message confusing. We can fix that by storing the parsed\n> message id in its own separate variable.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/receive-pack.c | 11 +++++++----\n>  fetch-pack.c           | 12 ++++++++----\n>  fsck.c                 |  8 ++++++--\n>  3 files changed, 21 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 8c4f0cb90a..ccf9738bce 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value)\n>  static int receive_pack_config(const char *var, const char *value,\n>  \t\t\t       const struct config_context *ctx, void *cb)\n>  {\n> +\tconst char *msg_id;\n>  \tint status = parse_hide_refs_config(var, value, \"receive\", &hidden_refs);\n>  \n>  \tif (status)\n> @@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tif (skip_prefix(var, \"receive.fsck.\", &var)) {\n> -\t\tif (is_valid_msg_type(var, value))\n> +\tif (skip_prefix(var, \"receive.fsck.\", &msg_id)) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tif (is_valid_msg_type(msg_id, value))\n>  \t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> -\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n> +\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n>  \t\telse\n> -\t\t\twarning(\"skipping unknown msg id '%s'\", var);\n> +\t\t\twarning(\"skipping unknown msg id '%s'\", msg_id);\n>  \t\treturn 0;\n>  \t}\n\nSame remark here. We would only generate warnings for an actual boolean,\nso I'd think we might consider doing the same for implicit booleans.\n\nPartick\n"},{"id":"485547","messageId":"20231212004649.GA376323@coredump.intra.peff.net","threadId":"60593","inReplyTo":"ZXF--AxCOOOjyOMc@tanuki","subject":"Re: [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T00:46:49Z","receivedAt":"2023-12-12T00:46:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2023 at 09:14:48AM +0100, Patrick Steinhardt wrote:\n\n> > diff --git a/submodule-config.c b/submodule-config.c\n> > index 6a48fd12f6..f4dd482abc 100644\n> > --- a/submodule-config.c\n> > +++ b/submodule-config.c\n> > @@ -516,7 +516,9 @@ static int parse_config(const char *var, const char *value,\n> >  \t\t\tsubmodule->recommend_shallow =\n> >  \t\t\t\tgit_config_bool(var, value);\n> >  \t} else if (!strcmp(item.buf, \"branch\")) {\n> > -\t\tif (!me->overwrite && submodule->branch)\n> > +\t\tif (!value)\n> > +\t\t\tret = config_error_nonbool(var);\n> > +\t\telse if (!me->overwrite && submodule->branch)\n> >  \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n> >  \t\t\t\t\t     \"branch\");\n> >  \t\telse {\n> \n> I was about to say that I'd rather expect us to handle this gracefully\n> so that Git doesn't die when parsing an invalid gitmodules file. But\n> there are other cases where we already fail in the same way, so this\n> looks good to me.\n\nWe're just returning the error here, so it's really up to the caller to\ndecide what to do. The config API has an \"error_action\" field in the\noptions struct. By default for reading from a blob, this will propagate\nthe error, and I think that's what we use in most of the submodule code.\n\nFor the code in fsck which looks at gitmodules, it suppresses the error\ntext from the config API (in favor of its own fsck-specific message).\nOf course it does not suppress the error() from config_error_nonbool,\nwhich writes straight to stderr. So there may be some room for\nimprovement, but I doubt anybody cares too much in practice if fsck is a\nlittle chatty when it sees breakage.\n\n-Peff\n"},{"id":"485548","messageId":"20231212005228.GB376323@coredump.intra.peff.net","threadId":"60593","inReplyTo":"ZXF-7AMZ_SBltplk@tanuki","subject":"Re: [PATCH 0/7] fix segfaults with implicit-bool config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T00:52:28Z","receivedAt":"2023-12-12T00:52:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote:\n\n> Thanks for working on this topic! I've left a couple of comments, most\n> of which are about whether we should retain previous behaviour where we\n> generate a warning instead of raising an error for unknown values.\n\nThanks for taking a look. I see what you're saying about the warnings,\nbut IMHO it's not worth the extra complexity. Returning early means the\nexisting code can proceed without worrying about NULLs. Though I suppose\nwe could have a \"warn_error_nonbool()\" which issues a warning and\nreturns 0.\n\nStill, the \"return config_error_nonbool()\" pattern is pretty\nwell-established and used for most options. I would go so far as to say\nthe ones that warn for invalid values are the odd ones out, and probably\nshould be returning errors as well (though doing so now may not be worth\nthe trouble and risk of annoyance).\n\nAnd certainly there should be no regressions in this series; every case\nis currently a segfault, so returning an error is a strict improvement.\nI'd just as soon stay strict there, as it's easier to loosen later if we\nchoose than to tighten.\n\n-Peff\n"},{"id":"485549","messageId":"20231212005807.GC376323@coredump.intra.peff.net","threadId":"60593","inReplyTo":"ZXF-8iNH0qaJSVl9@tanuki","subject":"Re: [PATCH 1/7] config: handle NULL value when parsing non-bools","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T00:58:07Z","receivedAt":"2023-12-12T00:58:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2023 at 09:14:42AM +0100, Patrick Steinhardt wrote:\n\n> >  \tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n> [...]\n> This isn't part of the diff and not a new issue, but why don't we\n> `return 0` when parsing this config correctly? We fall through to\n> `git_default_config()` even if we've successfully parsed the config key,\n> which seems like a bug to me.\n\nI don't think it's a functional bug, but merely a pessimization. We can\nreturn early if we know we've handled the option, but the rest of the\ncode would simply fail to match it. So we are just wasting a few strcmp\ncalls (and an unknown key already wastes the same number).\n\nSo I think it is a good practice to return, but not really a bug if we\ndon't.\n\n> >  \tif (!strcmp(var, \"core.checkstat\")) {\n> > +\t\tif (!value)\n> > +\t\t\treturn config_error_nonbool(var);\n> >  \t\tif (!strcasecmp(value, \"default\"))\n> >  \t\t\tcheck_stat = 1;\n> >  \t\telse if (!strcasecmp(value, \"minimal\"))\n> \n> We would ignore `true` here, so should we ignore implicit `true`, as\n> well?\n\nIMHO the lack of a final \"else\" in the strcasecmp if-cascade is a bug\n(and I sent a fix as part of the \"config fixes on top\" series). Even if\nwe want to leave it for historical reasons, I think it's still worth\nreturning an error for the NULL case (since we know it would have\nsegfaulted previously).\n\n(I snipped the rest of your mail, as I think my response to the cover\nletter covers the general discussion).\n\n-Peff\n"},{"id":"485558","messageId":"ZXfdGYUyJz4bnkQ0@tanuki","threadId":"60593","inReplyTo":"20231212005228.GB376323@coredump.intra.peff.net","subject":"Re: [PATCH 0/7] fix segfaults with implicit-bool config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T04:10:01Z","receivedAt":"2023-12-12T04:10:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 11, 2023 at 07:52:28PM -0500, Jeff King wrote:\n> On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote:\n> \n> > Thanks for working on this topic! I've left a couple of comments, most\n> > of which are about whether we should retain previous behaviour where we\n> > generate a warning instead of raising an error for unknown values.\n> \n> Thanks for taking a look. I see what you're saying about the warnings,\n> but IMHO it's not worth the extra complexity. Returning early means the\n> existing code can proceed without worrying about NULLs. Though I suppose\n> we could have a \"warn_error_nonbool()\" which issues a warning and\n> returns 0.\n> \n> Still, the \"return config_error_nonbool()\" pattern is pretty\n> well-established and used for most options. I would go so far as to say\n> the ones that warn for invalid values are the odd ones out, and probably\n> should be returning errors as well (though doing so now may not be worth\n> the trouble and risk of annoyance).\n> \n> And certainly there should be no regressions in this series; every case\n> is currently a segfault, so returning an error is a strict improvement.\n> I'd just as soon stay strict there, as it's easier to loosen later if we\n> choose than to tighten.\n\nFair enough, I'm perfectly fine with this reasoning. Thanks!\n\nPatrick\n"}]}