Re: [PATCH 1/3] pretty.c: fix null pointer dereference

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2026, 06:25 UTC
Message-ID: <xmqqcy1uk69o.fsf@gitster.g>
In-Reply-To: <20260224040400.751247-2-mroik@delayed.space>

Mirko Faina <mroik@delayed.space> writes:

Show 12 quoted lines

> commit_format_is_empty() is used to check whether "user_format" is set
> to a value. Unfortunately this function crashes the program if no
> user_format is set. This is because instead of checking for the pointer
> value it checks for its dereferenced value, this being NULL if
> user_format is not set.
>
> Teach the proper condition to check if user_format is set.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
>  pretty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Interesting.

Are any of the existing calls to this function that passes CMIT_FMT_USERFORMAT trigger a segfault with certain condition?

For example, there are only two places where CMIT_FMT_USERFORMAT is assigned to something. One is save_user_format() where user_format gets a non NULL string before rev->commit_format gets assigned CMIT_FMT_USERFORMAT. Another is git_pretty_formats_config() that parses configuration variables "pretty.*" and populate commit_formats map. This is later used in get_commit_format() and that function always calls save_user_format() we just saw when the format used is CMIT_FMT_USERFORMAT. So the existing code paths seem to be safe by design.

What I am wondering is if a NULL user_format should be flagged as a programming error, instead of getting swept under the rug like this patch does. IOW,

	int commit_format_is_empty(enum cmit_fmt fmt)
	{
		if (fmt != CMIT_FMT_USERFORMAT)
			return 0;
		if (!user_format)
			BUG("never called save_user_format() and using USERFORMAT?");
		return !*user_format;
	}

Show 14 quoted lines

>
> diff --git a/pretty.c b/pretty.c
> index e0646bbc5d..cdb8bf559d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -47,7 +47,7 @@ static struct cmt_fmt_map *find_commit_format(const char *sought);
>  
>  int commit_format_is_empty(enum cmit_fmt fmt)
>  {
> -	return fmt == CMIT_FMT_USERFORMAT && !*user_format;
> +	return fmt == CMIT_FMT_USERFORMAT && !user_format;
>  }
>  
>  static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)