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@xxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
