Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

> On Fri, Apr 28, 2017 at 12:41:02PM +0200, Johannes Schindelin wrote:
> 
> > But then, I guess I misunderstood what Coverity complained about:
> > maybe the problem was not so much the isspace() call but that EOF is
> > not being handled correctly. We pass it, unchecked, to ungetc().
> > 
> > It appears that I (or Coverity, if you will), missed another instance
> > where we simply passed EOF unchecked to ungetc().
> 
> I think that is also fine according to the standard.
> 
> Do you happen to have the exact error from Coverity?

Wow, that was unnecessarily hard. It is a major hassle to get to any
scan other than the latest one.

But I did it. Call me tenatious.

The report says this:

233        do {
   2. negative_return_fn: Function mingw_fgetc(f) returns a negative number.
   3. var_assign: Assigning: signed variable peek = mingw_fgetc.
234                peek = fgetc(f);
   CID 1049734: Negative array index read (NEGATIVE_RETURNS)
   4.  negative_returns: Using variable peek as an index to array sane_ctype.
235        } while (isspace(peek));
236        ungetc(peek, f);

So part of the thing is that we use mingw_fgetc() instead of fgetc().
However, the return value is *still* the one from the "real" fgetc(), even
if we intercept what appears to be a Ctrl+C from an interactive console.

> I'm wondering if it is complaining about some aspect of our custom
> isspace() when used with EOF.

That would appear to be the real issue, yes, and I should have
double-checked the claim that POSIX isspace() handles EOF properly: we
override isspace() with our own version, after all:

	#define isspace(x) sane_istest(x,GIT_SPACE)

where

	#define sane_istest(x,mask) \
		((sane_ctype[(unsigned char)(x)] & (mask)) != 0)

(rewrapped for readability)

As usual, EOF is defined as -1 in Git for Windows' context, meaning that
we look at the last entry of the sane_ctype array, which returns 0 for any
sane_istest(x,mask) test for x >= 0x80:

        /* Nothing in the 128.. range */

So it would appear that it happens to work, but I doubt that it was
intentional.

Having said that, it is really curious why Coverity should get confused by
the code and not realize that casting a negative number to (unsigned char)
will make it valid as an index for the sane_ctype array.

I double-checked, and there is no override for the isspace() function in
what Coverity calls a "model file" (i.e. pseudo code intended to helping
Coverity realize where it can stop reporting false positives).

> > The next iteration will have it completely reworked: I no longer guard
> > the isspace() behind an `!= EOF` check, but rather handle an early EOF
> > as I think it should be handled. Extra eyes very welcome (this is the
> > fixup!  patch):
> 
> I do think handling EOF explicitly is probably a better strategy anyway,
> as it lets us tell when we have an empty patch.

I agree, I came to the same conclusion independently.

Ciao,
Dscho