Re: [PATCH] entry: check for fstat() errors after checkout

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2020, 14:08 UTC
Message-ID: <xmqqr1tkye5g.fsf@gitster.c.googlers.com>
In-Reply-To: <882592a4-ffb7-924c-b52a-3287b11318b8@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 18 quoted lines

> On 7/8/2020 10:10 PM, Matheus Tavares wrote:
>> In 11179eb311 ("entry.c: check if file exists after checkout",
>> 2017-10-05) we started checking the result of the lstat() call done
>> after writing a file, to avoid writing garbage to the corresponding
>> cache entry. However, the code skips calling lstat() if it's possible
>> to use fstat() when it still has the file descriptor open. And when
>> calling fstat() we don't do the same error checking. To fix that, let
>> the callers of fstat_output() know when fstat() fails. In this case,
>> write_entry() will try to use lstat() and properly report an error if
>> that fails as well.
>
> Looking at this for the first time, I was confused because 11179eb311
> doesn't touch these lines. But that's the point: it should have.
>
> Thanks for finding this! I wonder if there is a way to expose this
> behavior in a test... it definitely seems like this is only something
> that happens if there is a failure in the filesystem, so I'm not sure
> such a thing is possible.

If another process removed the path from the filesystem after this process created it and before this codepath used fstat() on it, fstat() may succeed while lstat() would definitely fail. There is no "failure in the filesystem", but it would be harder to arrange.

> It would just be nice to know the ramifications of this change in
> behavior, keeping in mind that this behavior started way back in
> e4c7292353 (write_entry(): use fstat() instead of lstat() when file is
> open, 2009-02-09), over 11 years ago!

Yup.  I am curious how this was found ;-)

Thanks.