Re: lstat-ing delayed-filter output, was Re: playing with MSan
- Date: Thu, 5 Oct 2017 12:47:13 +0200
- From: Lars Schneider <larsxschneider@xxxxxxxxx>
- Subject: Re: lstat-ing delayed-filter output, was Re: playing with MSan
> On 05 Oct 2017, at 05:46, Jeff King <peff@xxxxxxxx> wrote:
> On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:
>>> 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.
Oh. Great catch!
> 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.
That sounds good to me!
I tried to address both issues here: