ERANGE strikes again on my Windows build; RFH

> 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.
>
> What if a subordinate callee starts doing
>
> 	if (some_syscall(...) < 0) {
> 		if (errno == EEXIST) {
> 			/* never mind, that's OK */
> 			...
> 		}
> 	}
>
> Would it be required to reset errno to its previous value in this
> failure-is-not-an-error case?
>
> The problem in my Windows build is that one of these subordinate
> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
> and it fails with ERANGE when the buffer is too short. Do I have to
> modify the vsnprintf emulation to restore errno?

> On Sat, Dec 28, 2019 at 04:41:42PM +0100, 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.
>>
>> What if a subordinate callee starts doing
>>
>> 	if (some_syscall(...) < 0) {
>> 		if (errno == EEXIST) {
>> 			/* never mind, that's OK */
>> 			...
>> 		}
>> 	}
>>
>> Would it be required to reset errno to its previous value in this
>> failure-is-not-an-error case?
>>
>> The problem in my Windows build is that one of these subordinate
>> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
>> and it fails with ERANGE when the buffer is too short. Do I have to
>> modify the vsnprintf emulation to restore errno?
> 
> If you ask me: I think so, yes.
> At least the documentation about vsnprintf does not mention that errno is touched at all.
> That is the man pages for Linux and Mac OS, or see here:
> https://linux.die.net/man/3/vsnprintf
> 
> It would make sense to analyze the complete callstack, I think.
> Is your problem reproducable ?
> 
> Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.
> 

> Le 29/12/2019 à 15:29, Torsten Bögershausen a écrit :
>> On Sat, Dec 28, 2019 at 04:41:42PM +0100, 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.
>>>
>>> What if a subordinate callee starts doing
>>>
>>> 	if (some_syscall(...) < 0) {
>>> 		if (errno == EEXIST) {
>>> 			/* never mind, that's OK */
>>> 			...
>>> 		}
>>> 	}
>>>
>>> Would it be required to reset errno to its previous value in this
>>> failure-is-not-an-error case?
>>>
>>> The problem in my Windows build is that one of these subordinate
>>> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),
>>> and it fails with ERANGE when the buffer is too short. Do I have to
>>> modify the vsnprintf emulation to restore errno?
>>
>> If you ask me: I think so, yes.
>> At least the documentation about vsnprintf does not mention that errno is touched at all.
>> That is the man pages for Linux and Mac OS, or see here:
>> https://linux.die.net/man/3/vsnprintf
>>
>> It would make sense to analyze the complete callstack, I think.
>> Is your problem reproducable ?
>>
>> Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.
>>
> 
> According to the standard, vsnprintf() _can_ change errno[1] (and the
> BSDs do so[2][3][4].)  But apparently, not to ERANGE.

> 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.

> What if a subordinate callee starts doing
>
> 	if (some_syscall(...) < 0) {
> 		if (errno == EEXIST) {
> 			/* never mind, that's OK */
> 			...
> 		}
> 	}
>
> Would it be required to reset errno to its previous value in this
> failure-is-not-an-error case?

> 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.

> 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

>  (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.

>>                                                                    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.

>> 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:

> Johannes Sixt wrote:
> > Am 30.12.19 um 19:06 schrieb Jonathan Nieder:
> 
> >>                                                                    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.
> 
> On the contrary: this caller is using errno as an error *indicator*
> instead of a way of *distinguishing* between errors (or to put it
> another way, this caller is treating `errno == 0` as a meaningful
> condition).  This means the calling code is buggy.