Re: [PATCH 7/7] fsck: handle NULL value when parsing message config

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Thu, Dec 07, 2023 at 02:11:35AM -0500, Jeff King wrote:
> 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 <peff@peff.net>
> ---
>  builtin/receive-pack.c | 11 +++++++----
>  fetch-pack.c           | 12 ++++++++----
>  fsck.c                 |  8 ++++++--
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> 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;
>  	}

Same remark here. We would only generate warnings for an actual boolean,
so I'd think we might consider doing the same for implicit booleans.

Partick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]