Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schinde...@gmx.de> writes:
>
> > 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.
I changed it to error out with a (translated) "empty mbox: '%s'" as the
other hunks.
> > @@ -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?
Sorry, I *assumed* that the function was passed a zero-initialized
mailinfo struct. And as you pointed out, that assumption was wrong.
My thinking was that I did not want to introduce another leakage by my
patch, but as your careful analysis determined: the only caller that does
not die() afterwards releases the data (and would not even be able to
handle p_hdr_data == NULL...).
I reverted that move.
Ciao,
Dscho
