Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 2, 2017, 13:57 UTC
- Message-ID
- <alpine.DEB.2.20.1705021553420.3480@virtualbox>
- In-Reply-To
- <xmqqzievdhk5.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
Show 18 quoted lines
> Johannes Schindelin <johannes.schindelin@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.
Show 18 quoted lines
> > @@ -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