Re: ERANGE strikes again on my Windows build; RFH

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 30, 2019, 18:46 UTC
Message-ID: <54522fee-0796-df46-a3cf-4331537ecf59@kdbg.org>
In-Reply-To: <20191230180653.GA57251@google.com>

Am 30.12.19 um 19:06 schrieb Jonathan Nieder:

Show 25 quoted lines

> Johannes Sixt wrote:
> 
>> In sha1-file.c:read_object_file_extended() we have the following pattern:
>>
>> 	errno = 0;
>> 	data = read_object(r, repl, type, size);
>> 	if (data)
>> 		return data;
>>
>> 	if (errno && errno != ENOENT)
>> 		die_errno(_("failed to read object %s"), oid_to_hex(oid));
>>
>> That is, it is expected that read_object() does not change the value of
>> errno in the non-error case. I find it intriguing that we expect a quite
>> large call graph that is behind read_object() to behave this way.
> 
> Yes, this seems dubious.
> 
> In fact this is only inspecting errno in the returned-NULL case.  If I
> look only at the code above and not at the implementation of
> read_object, then I would say that the bug is the 'errno &&' part: when
> errno is meaningful for a function for a given return value, the usual
> convention is
> 
>  (1) it *always* sets errno for errors, not conditionally

You seem to understand that errno isn't set somewhere where it should be set. But the problem is not absence of setting errno, but abundance of setting errno; in particular, when functions receive errors from lower-level functions, but then indicate to the higher levels that everything's fine; then the higher levels observe errno when they shouldn't.

Show 8 quoted lines

>  (2) it never sets errno to 0
> 
> There are some exceptions (like strtoul) but they are few and
> unfortunate, not something to be imitated.
> 
> Do you have more details about the case where read_object is expected
> to produce errno == 0?  I'm wondering whether we forgot to set 'errno
> = ENOENT' explicitly somewhere.

I don't think that forgetting to set ENOENT is the problem.

It happens reproducibly in test 5 of t0410-partial-clone:

    ++ git -C repo fsck
    Checking object directories: 100% (256/256), done.
    Checking objects: 100% (1/1), done.
    fatal: failed to read object
383670739c2f993999f3c10911ac5cb5c6591523: Result too large

when it should be

    Checking object directories: 100% (256/256), done.
    Checking objects: 100% (1/1), done.
    dangling tag e5f4cb9fd329c512b08fb81a8e6b1f5e27658263

-- Hannes