From ba176db511b3438738a4aeb98e574310e697ff5f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:14 -0500 Subject: config: handle NULL value when parsing non-bools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/blame.c | 2 ++ builtin/checkout.c | 2 ++ builtin/clone.c | 2 ++ builtin/log.c | 5 ++++- builtin/pack-objects.c | 6 +++++- 5 files changed, 15 insertions(+), 2 deletions(-) (limited to 'builtin') 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,7 +3204,7 @@ 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 @@ -3212,6 +3212,10 @@ static int git_pack_config(const char *k, const char *v, */ 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) || -- cgit v1.3-5-g9baa From d49cb162fa752d62cf20548ae057471d348e42ae Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 7 Dec 2023 02:11:35 -0500 Subject: fsck: handle NULL value when parsing message config When parsing fsck.*, receive.fsck.*, or fetch.fsck.*, we don't check for an implicit bool. So any of: [fsck] badTree [receive "fsck"] badTree [fetch "fsck"] badTree will cause us to segfault. We can fix it with config_error_nonbool() in the usual way, but we have to make a few more changes to get good error messages. The problem is that all three spots do: if (skip_prefix(var, "fsck.", &var)) to match and parse the actual message id. But that means that "var" now just says "badTree" instead of "receive.fsck.badTree", making the resulting message confusing. We can fix that by storing the parsed message id in its own separate variable. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 11 +++++++---- fetch-pack.c | 12 ++++++++---- fsck.c | 8 ++++++-- 3 files changed, 21 insertions(+), 10 deletions(-) (limited to 'builtin') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 8c4f0cb90a..ccf9738bce 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -142,6 +142,7 @@ static enum deny_action parse_deny_action(const char *var, const char *value) static int receive_pack_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { + const char *msg_id; int status = parse_hide_refs_config(var, value, "receive", &hidden_refs); if (status) @@ -178,12 +179,14 @@ static int receive_pack_config(const char *var, const char *value, return 0; } - if (skip_prefix(var, "receive.fsck.", &var)) { - if (is_valid_msg_type(var, value)) + if (skip_prefix(var, "receive.fsck.", &msg_id)) { + if (!value) + return config_error_nonbool(var); + if (is_valid_msg_type(msg_id, value)) strbuf_addf(&fsck_msg_types, "%c%s=%s", - fsck_msg_types.len ? ',' : '=', var, value); + fsck_msg_types.len ? ',' : '=', msg_id, value); else - warning("skipping unknown msg id '%s'", var); + warning("skipping unknown msg id '%s'", msg_id); return 0; } diff --git a/fetch-pack.c b/fetch-pack.c index 26999e3b65..31a72d43de 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1862,6 +1862,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, static int fetch_pack_config_cb(const char *var, const char *value, const struct config_context *ctx, void *cb) { + const char *msg_id; + if (strcmp(var, "fetch.fsck.skiplist") == 0) { const char *path; @@ -1873,12 +1875,14 @@ static int fetch_pack_config_cb(const char *var, const char *value, return 0; } - if (skip_prefix(var, "fetch.fsck.", &var)) { - if (is_valid_msg_type(var, value)) + if (skip_prefix(var, "fetch.fsck.", &msg_id)) { + if (!value) + return config_error_nonbool(var); + if (is_valid_msg_type(msg_id, value)) strbuf_addf(&fsck_msg_types, "%c%s=%s", - fsck_msg_types.len ? ',' : '=', var, value); + fsck_msg_types.len ? ',' : '=', msg_id, value); else - warning("Skipping unknown msg id '%s'", var); + warning("Skipping unknown msg id '%s'", msg_id); return 0; } diff --git a/fsck.c b/fsck.c index 6a0bbc5087..b624083a13 100644 --- a/fsck.c +++ b/fsck.c @@ -1403,6 +1403,8 @@ int git_fsck_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { struct fsck_options *options = cb; + const char *msg_id; + if (strcmp(var, "fsck.skiplist") == 0) { const char *path; struct strbuf sb = STRBUF_INIT; @@ -1416,8 +1418,10 @@ int git_fsck_config(const char *var, const char *value, return 0; } - if (skip_prefix(var, "fsck.", &var)) { - fsck_set_msg_type(options, var, value); + if (skip_prefix(var, "fsck.", &msg_id)) { + if (!value) + return config_error_nonbool(var); + fsck_set_msg_type(options, msg_id, value); return 0; } -- cgit v1.3-5-g9baa