From: Mirko Faina <mroik@delayed.space>
Date: Tue, 24 Feb 2026 07:08:35 GMT
Subject: Re: [PATCH 1/3] pretty.c: fix null pointer dereference
Message-ID: <aZ1JND7sGspCJEoc@exploit>
In-Reply-To: <xmqqcy1uk69o.fsf@gitster.g>

On Mon, Feb 23, 2026 at 10:25:07PM -0800, Junio C Hamano wrote:
> 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;
> 	}

This doesn't convince me, I think it is a bug. If dereferencing NULL was
done on purpose why not use die() instead? Also, the only way for us to
check user_format is through commit_format_is_empty() as it is static.
In a complex config setup it might be useful to double check just to be
sure, and I wouldn't want the program to crash on a failed check.

save_user_format() is not available neither, so evaluation of the format
string has to be done through get_commit_format(). If I pass any of the
predefined formats (CMIT_FMT_*) get_commit_format() won't set
user_format. So the only way for us to check if it was set is through
commit_format_is_empty() (well technically there's
rev_info->commit_format but it still doesn't feel like it was
intentional).

But if the intended behaviour was for the program to crash then I take
issue with the name.