| Message ID | 20231207071114.GA1276005@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix segfaults with implicit-bool config | expand |
On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote: > When the config parser sees an "implicit" bool like: > > [core] > someVariable > > it passes NULL to the config callback. Any callback code which expects a > string must check for NULL. This usually happens via helpers like > git_config_string(), etc, but some custom code forgets to do so and will > segfault. > > These are all fairly vanilla cases where the solution is just the usual > pattern of: > > if (!value) > return config_error_nonbool(var); > > though note that in a few cases we have to split initializers like: > > int some_var = initializer(); > > into: > > int some_var; > if (!value) > return config_error_nonbool(var); > some_var = initializer(); > > There are still some broken instances after this patch, which I'll > address on their own in individual patches after this one. > > Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/blame.c | 2 ++ > builtin/checkout.c | 2 ++ > builtin/clone.c | 2 ++ > builtin/log.c | 5 ++++- > builtin/pack-objects.c | 6 +++++- > compat/mingw.c | 2 ++ > config.c | 8 ++++++++ > diff.c | 19 ++++++++++++++++--- > mailinfo.c | 2 ++ > notes-utils.c | 2 ++ > trailer.c | 2 ++ > 11 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/builtin/blame.c b/builtin/blame.c > index 9c987d6567..2433b7da5c 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value, > } > > if (!strcmp(var, "blame.coloring")) { > + if (!value) > + return config_error_nonbool(var); In the `else` statement where we fail to parse the value we only generate a warning and return successfully regardless. Should we do the same here? > if (!strcmp(value, "repeatedLines")) { > coloring_mode |= OUTPUT_COLOR_LINE; > } else if (!strcmp(value, "highlightRecent")) { > diff --git a/builtin/checkout.c b/builtin/checkout.c > index f02434bc15..d5c784854f 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value, > struct checkout_opts *opts = cb; > > if (!strcmp(var, "diff.ignoresubmodules")) { > + if (!value) > + return config_error_nonbool(var); > handle_ignore_submodules_arg(&opts->diff_options, value); > return 0; > } This one is fine, `handle_ignore_submodules_arg()` dies if it gets an unknown value and `git_config()` will die when the callback function returns an error. The same is true for many other cases you've converted. [snip] > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 89a8b5a976..62c540b4db 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v, > return 0; > } > if (!strcmp(k, "uploadpack.blobpackfileuri")) { > - struct configured_exclusion *ex = xmalloc(sizeof(*ex)); > + struct configured_exclusion *ex; > const char *oid_end, *pack_end; > /* > * Stores the pack hash. This is not a true object ID, but is > * of the same form. > */ > struct object_id pack_hash; > > + if (!v) > + return config_error_nonbool(k); > + > + ex = xmalloc(sizeof(*ex)); > if (parse_oid_hex(v, &ex->e.oid, &oid_end) || > *oid_end != ' ' || > parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) || This isn't part of the diff and not a new issue, but why don't we `return 0` when parsing this config correctly? We fall through to `git_default_config()` even if we've successfully parsed the config key, which seems like a bug to me. Anyway, this case looks fine. [snip] > diff --git a/config.c b/config.c > index b330c7adb4..18085c7e38 100644 > --- a/config.c > +++ b/config.c > @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value, > return 0; > } > if (!strcmp(var, "core.checkstat")) { > + if (!value) > + return config_error_nonbool(var); > if (!strcasecmp(value, "default")) > check_stat = 1; > else if (!strcasecmp(value, "minimal")) We would ignore `true` here, so should we ignore implicit `true`, as well? > @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value, > } > > if (!strcmp(var, "core.checkroundtripencoding")) { > + if (!value) > + return config_error_nonbool(var); > check_roundtrip_encoding = xstrdup(value); > return 0; > } > > if (!strcmp(var, "core.notesref")) { > + if (!value) > + return config_error_nonbool(var); > notes_ref_name = xstrdup(value); > return 0; > } I wonder the same here. We might as well use `xstrdup_or_null()`, but it feels like the right thing to do to convert these to actual errors. > @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value, > if (!strcmp(var, "diff.orderfile")) > return git_config_pathname(&diff_order_file_cfg, var, value); > > - if (!strcmp(var, "diff.ignoresubmodules")) > + if (!strcmp(var, "diff.ignoresubmodules")) { > + if (!value) > + return config_error_nonbool(var); > handle_ignore_submodules_arg(&default_diff_options, value); > + } > > if (!strcmp(var, "diff.submodule")) { > + if (!value) > + return config_error_nonbool(var); > if (parse_submodule_params(&default_diff_options, value)) > warning(_("Unknown value for 'diff.submodule' config variable: '%s'"), > value); Should we generate a warning instead according to the preexisting code for "diff.submodule"? > @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value, > > if (!strcmp(var, "diff.dirstat")) { > struct strbuf errmsg = STRBUF_INIT; > + if (!value) > + return config_error_nonbool(var); > default_diff_options.dirstat_permille = diff_dirstat_permille_default; > if (parse_dirstat_params(&default_diff_options, value, &errmsg)) > warning(_("Found errors in 'diff.dirstat' config variable:\n%s"), Same here, should we generate a warning instead? > diff --git a/notes-utils.c b/notes-utils.c > index 97c031c26e..01f4f5b424 100644 > --- a/notes-utils.c > +++ b/notes-utils.c > @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v, > } > return 0; > } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) { > + if (!v) > + return config_error_nonbool(k); > /* note that a refs/ prefix is implied in the > * underlying for_each_glob_ref */ > if (starts_with(v, "refs/notes/")) Here, as well. > diff --git a/trailer.c b/trailer.c > index b6de5d9cb2..b0e2ec224a 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, > warning(_("unknown value '%s' for key '%s'"), > value, conf_key); > } else if (!strcmp(trailer_item, "separators")) { > + if (!value) > + return config_error_nonbool(conf_key); > separators = xstrdup(value); > } > } And here. Patrick
On Thu, Dec 07, 2023 at 09:14:42AM +0100, Patrick Steinhardt wrote: > > if (!strcmp(k, "uploadpack.blobpackfileuri")) { > [...] > This isn't part of the diff and not a new issue, but why don't we > `return 0` when parsing this config correctly? We fall through to > `git_default_config()` even if we've successfully parsed the config key, > which seems like a bug to me. I don't think it's a functional bug, but merely a pessimization. We can return early if we know we've handled the option, but the rest of the code would simply fail to match it. So we are just wasting a few strcmp calls (and an unknown key already wastes the same number). So I think it is a good practice to return, but not really a bug if we don't. > > if (!strcmp(var, "core.checkstat")) { > > + if (!value) > > + return config_error_nonbool(var); > > if (!strcasecmp(value, "default")) > > check_stat = 1; > > else if (!strcasecmp(value, "minimal")) > > We would ignore `true` here, so should we ignore implicit `true`, as > well? IMHO the lack of a final "else" in the strcasecmp if-cascade is a bug (and I sent a fix as part of the "config fixes on top" series). Even if we want to leave it for historical reasons, I think it's still worth returning an error for the NULL case (since we know it would have segfaulted previously). (I snipped the rest of your mail, as I think my response to the cover letter covers the general discussion). -Peff
diff --git a/builtin/blame.c b/builtin/blame.c index 9c987d6567..2433b7da5c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value, } if (!strcmp(var, "blame.coloring")) { + if (!value) + return config_error_nonbool(var); if (!strcmp(value, "repeatedLines")) { coloring_mode |= OUTPUT_COLOR_LINE; } else if (!strcmp(value, "highlightRecent")) { diff --git a/builtin/checkout.c b/builtin/checkout.c index f02434bc15..d5c784854f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value, struct checkout_opts *opts = cb; if (!strcmp(var, "diff.ignoresubmodules")) { + if (!value) + return config_error_nonbool(var); handle_ignore_submodules_arg(&opts->diff_options, value); return 0; } diff --git a/builtin/clone.c b/builtin/clone.c index c6357af949..54d9b9976a 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -791,6 +791,8 @@ static int git_clone_config(const char *k, const char *v, const struct config_context *ctx, void *cb) { if (!strcmp(k, "clone.defaultremotename")) { + if (!v) + return config_error_nonbool(k); free(remote_name); remote_name = xstrdup(v); } diff --git a/builtin/log.c b/builtin/log.c index ba775d7b5c..3ce41c4856 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -594,8 +594,11 @@ static int git_log_config(const char *var, const char *value, decoration_style = 0; /* maybe warn? */ return 0; } - if (!strcmp(var, "log.diffmerges")) + if (!strcmp(var, "log.diffmerges")) { + if (!value) + return config_error_nonbool(var); return diff_merges_config(value); + } if (!strcmp(var, "log.showroot")) { default_show_root = git_config_bool(var, value); return 0; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 89a8b5a976..62c540b4db 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v, return 0; } if (!strcmp(k, "uploadpack.blobpackfileuri")) { - struct configured_exclusion *ex = xmalloc(sizeof(*ex)); + struct configured_exclusion *ex; const char *oid_end, *pack_end; /* * Stores the pack hash. This is not a true object ID, but is * of the same form. */ struct object_id pack_hash; + if (!v) + return config_error_nonbool(k); + + ex = xmalloc(sizeof(*ex)); if (parse_oid_hex(v, &ex->e.oid, &oid_end) || *oid_end != ' ' || parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) || diff --git a/compat/mingw.c b/compat/mingw.c index ec5280da16..42053c1f65 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -255,6 +255,8 @@ int mingw_core_config(const char *var, const char *value, } if (!strcmp(var, "core.unsetenvvars")) { + if (!value) + return config_error_nonbool(var); free(unset_environment_variables); unset_environment_variables = xstrdup(value); return 0; diff --git a/config.c b/config.c index b330c7adb4..18085c7e38 100644 --- a/config.c +++ b/config.c @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value, return 0; } if (!strcmp(var, "core.checkstat")) { + if (!value) + return config_error_nonbool(var); if (!strcasecmp(value, "default")) check_stat = 1; else if (!strcasecmp(value, "minimal")) @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value, } if (!strcmp(var, "core.checkroundtripencoding")) { + if (!value) + return config_error_nonbool(var); check_roundtrip_encoding = xstrdup(value); return 0; } if (!strcmp(var, "core.notesref")) { + if (!value) + return config_error_nonbool(var); notes_ref_name = xstrdup(value); return 0; } @@ -1619,6 +1625,8 @@ static int git_default_core_config(const char *var, const char *value, } if (!strcmp(var, "core.createobject")) { + if (!value) + return config_error_nonbool(var); if (!strcmp(value, "rename")) object_creation_mode = OBJECT_CREATION_USES_RENAMES; else if (!strcmp(value, "link")) diff --git a/diff.c b/diff.c index 2c602df10a..5b213a4b44 100644 --- a/diff.c +++ b/diff.c @@ -372,7 +372,10 @@ int git_diff_ui_config(const char *var, const char *value, return 0; } if (!strcmp(var, "diff.colormovedws")) { - unsigned cm = parse_color_moved_ws(value); + unsigned cm; + if (!value) + return config_error_nonbool(var); + cm = parse_color_moved_ws(value); if (cm & COLOR_MOVED_WS_ERROR) return -1; diff_color_moved_ws_default = cm; @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value, if (!strcmp(var, "diff.orderfile")) return git_config_pathname(&diff_order_file_cfg, var, value); - if (!strcmp(var, "diff.ignoresubmodules")) + if (!strcmp(var, "diff.ignoresubmodules")) { + if (!value) + return config_error_nonbool(var); handle_ignore_submodules_arg(&default_diff_options, value); + } if (!strcmp(var, "diff.submodule")) { + if (!value) + return config_error_nonbool(var); if (parse_submodule_params(&default_diff_options, value)) warning(_("Unknown value for 'diff.submodule' config variable: '%s'"), value); @@ -473,7 +481,10 @@ int git_diff_basic_config(const char *var, const char *value, } if (!strcmp(var, "diff.wserrorhighlight")) { - int val = parse_ws_error_highlight(value); + int val; + if (!value) + return config_error_nonbool(var); + val = parse_ws_error_highlight(value); if (val < 0) return -1; ws_error_highlight_default = val; @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value, if (!strcmp(var, "diff.dirstat")) { struct strbuf errmsg = STRBUF_INIT; + if (!value) + return config_error_nonbool(var); default_diff_options.dirstat_permille = diff_dirstat_permille_default; if (parse_dirstat_params(&default_diff_options, value, &errmsg)) warning(_("Found errors in 'diff.dirstat' config variable:\n%s"), diff --git a/mailinfo.c b/mailinfo.c index a07d2da16d..093bed5d8f 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -1253,6 +1253,8 @@ static int git_mailinfo_config(const char *var, const char *value, return 0; } if (!strcmp(var, "mailinfo.quotedcr")) { + if (!value) + return config_error_nonbool(var); if (mailinfo_parse_quoted_cr_action(value, &mi->quoted_cr) != 0) return error(_("bad action '%s' for '%s'"), value, var); return 0; diff --git a/notes-utils.c b/notes-utils.c index 97c031c26e..01f4f5b424 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v, } return 0; } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) { + if (!v) + return config_error_nonbool(k); /* note that a refs/ prefix is implied in the * underlying for_each_glob_ref */ if (starts_with(v, "refs/notes/")) diff --git a/trailer.c b/trailer.c index b6de5d9cb2..b0e2ec224a 100644 --- a/trailer.c +++ b/trailer.c @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, warning(_("unknown value '%s' for key '%s'"), value, conf_key); } else if (!strcmp(trailer_item, "separators")) { + if (!value) + return config_error_nonbool(conf_key); separators = xstrdup(value); } }
When the config parser sees an "implicit" bool like: [core] someVariable it passes NULL to the config callback. Any callback code which expects a string must check for NULL. This usually happens via helpers like git_config_string(), etc, but some custom code forgets to do so and will segfault. These are all fairly vanilla cases where the solution is just the usual pattern of: if (!value) return config_error_nonbool(var); though note that in a few cases we have to split initializers like: int some_var = initializer(); into: int some_var; if (!value) return config_error_nonbool(var); some_var = initializer(); There are still some broken instances after this patch, which I'll address on their own in individual patches after this one. Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- builtin/blame.c | 2 ++ builtin/checkout.c | 2 ++ builtin/clone.c | 2 ++ builtin/log.c | 5 ++++- builtin/pack-objects.c | 6 +++++- compat/mingw.c | 2 ++ config.c | 8 ++++++++ diff.c | 19 ++++++++++++++++--- mailinfo.c | 2 ++ notes-utils.c | 2 ++ trailer.c | 2 ++ 11 files changed, 47 insertions(+), 5 deletions(-)