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

[Junio C Hamano <gitster@pobox.com>]

Mirko Faina <mroik@delayed.space> writes:

> 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;
	}





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