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
