Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> While POSIX states that it is okay to pass EOF to isspace() (and it seems
> to be implied that EOF should *not* be treated as whitespace), and also to
> pass EOF to ungetc() (which seems to be intended to fail without buffering
> the character), it is much better to handle these cases explicitly. Not
> only does it reduce head-scratching (and helps static analysis avoid
> reporting false positives), it also lets us handle files containing
> nothing but whitespace by erroring out.
>
> Reported via Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/mailsplit.c |  3 ++-
>  mailinfo.c          | 15 +++++++++++----
>  2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 30681681c13..9b3efc8e987 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
>  	do {
>  		peek = fgetc(f);
>  	} while (isspace(peek));
> -	ungetc(peek, f);
> +	if (peek != EOF)
> +		ungetc(peek, f);

I agree more with the first sentence in the proposed log message
than what this code actually does.  I.e. breaking upon seeing an EOF
explicitly would be nice, just like the change to mailinfo.c in this
patch we see below.

> @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)
>  		return -1;
>  	}
>  
> -	mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> -	mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> -
>  	do {
>  		peek = fgetc(mi->input);
> +		if (peek == EOF) {
> +			fclose(cmitmsg);
> +			return error("empty patch: '%s'", patch);
> +		}
>  	} while (isspace(peek));
>  	ungetc(peek, mi->input);

The handling of EOF is improved, but is it correct to move the
allocation of p/s_hdr_data down?

Among the two callers, builtin/am.c just dies when it sees
mailinfo() returns an error, but builtin/mailinfo.c tries to be
nicer and calls clear_mailinfo().  Wouldn't this make that codepath
dereference a NULL pointer?

I think the moral of the story is that people tend to get sloppier
when doing "while we are at it" than the main task, and a reviewer
needs to be more careful while reviewing the "while we are at it"
part of the change than the primary thing a patch wants to do ;-)

> +	mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> +	mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> +
>  	/* process the email header */
>  	while (read_one_header_line(&line, mi->input))
>  		check_header(mi, &line, mi->p_hdr_data, 1);