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

[Jeff King <peff@peff.net>]

On Tue, Feb 24, 2026 at 08:08:35AM +0100, Mirko Faina wrote:

> 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.

I am not quite sure what you are asking. No, the intent of that function
is not to crash. It is to check whether the user passed us an empty
string. Like the "git diff-tree --format= $commit" example given
in the commit message of b9c7d6e433 (pretty: make empty userformats
truly empty, 2014-07-29). If they did, then the first character of the
string will be the NUL terminator.

So dropping the "*" as your patch 1/3 does is just wrong. It is losing
the check for an empty string and replacing it with a check for a NULL
pointer.

The user_format string should never be NULL if we are using
CMIT_FMT_USERFORMAT. That's not checked for explicitly here, but is an
assumption of the pretty.c code. If there's some way to violate that
assumption, that's a bug (but it sounds from digging that there isn't).

If you have _new_ code which is using CMIT_FMT_USERFORMAT without
setting user_format to a non-NULL value, we might need to work around
that assumption. But I think what your 2/3 is doing is not quite at the
right level, which is the source of the trouble. I'll respond separately
to that patch.

-Peff