lstat-ing delayed-filter output, was Re: playing with MSan

From: Jeff King <peff@peff.net>
Date: Oct 5, 2017, 03:46 UTC
Message-ID: <20171005034632.kzsspk7wsuk23kf2@sigill.intra.peff.net>
In-Reply-To: <20171004193005.GD30301@hank>

On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:

Show 9 quoted lines

> > So I dunno. This approach is a _lot_ more convenient than trying to
> > rebuild all the dependencies from scratch, and it runs way faster than
> > valgrind.  It did find the cases that led to the patches in this
> > series, and at least one more: if the lstat() at the end of
> > entry.c:write_entry() fails, we write nonsense into the cache_entry.
> 
> Yeah valgrind found that one too, as I tried (and apparently failed :))
> to explain in the cover letter.  I just haven't found the time yet to
> actually try and go fix that one.

No, I just have poor memory. :)

The obvious fix is that we should check the return value of `lstat`, but the bigger question is why and when that would fail.

The case triggered by t0021 is using the new "delayed" filter mechanism. So at the time that write_entry() finishes, we don't actually have the file in the filesystem. I think we need to recognize that we got delayed and didn't actually check anything out, and skip that whole "if (state->refresh_cache)" block. It's not clear to me, though, how we tell the difference between the delayed and normal cases in that function.

But I think this lstat could also fail if we are checking out and somebody else racily deletes our file. This is presumably sufficiently rare that I actually wonder if we should just bail with an error, so that the user knows something funny is going on.

+cc Lars for thoughts no the delayed-filter case.

-Peff