ERANGE strikes again on my Windows build; RFH

ERANGE strikes again on my Windows build; RFH

[Johannes Sixt]

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?

Re: ERANGE strikes again on my Windows build; RFH

[Torsten Bögershausen]

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.

Re: ERANGE strikes again on my Windows build; RFH

[Andreas Schwab]

On Dez 29 2019, Torsten Bögershausen wrote:

> 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

Refer to the POSIX docs for vsnprintf, which documents its errno usage.
In general, any library call can touch errno, unless it is explicitly
documented _not_ to touch it.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: ERANGE strikes again on my Windows build; RFH

[Alban Gruin]

Hi,

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.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/vfprintf.html
[2]
https://www.freebsd.org/cgi/man.cgi?query=vsnprintf&manpath=FreeBSD+12.1-RELEASE
[3] https://www.freebsd.org/cgi/man.cgi?query=vsnprintf&manpath=NetBSD+8.1
[4] https://man.openbsd.org/vsnprintf

Cheers,
Alban

Re: ERANGE strikes again on my Windows build; RFH

[Johannes Sixt]

Am 29.12.19 um 18:25 schrieb Alban Gruin:
> 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.

I am not worried about errno being set (or to what value) when there
actually is an error. I am asking what to do when there is actually *no*
error. In my vsnprintf emulation, the case where ERANGE happens is *not*
an error as far as the emulation is concerned.

What if in the huge call graph behind read_object() some function
changes errno to, say, EEXIST, EISDIR, or ENODIR and the condition under
which this happens is *not* an error in that context? Is the function
required to restore the original errno?

Consider the task to create file "foo/bar.c". We would have to
mkdir("foo"), but it is *not* an error when mkdir() fails with errno ==
EEXIST. Are we required to reset errno back to its old value?

(I know, read_object() is unlikely to allocate files, but I think I have
to explain in some way that the context may define that there is no
error -- even though a lower-level function failed and modified errno.)

-- Hannes

Re: ERANGE strikes again on my Windows build; RFH

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

Yeah, that is somewhat unfortunate and dubious, but as long as it
works (i.e. various system library calls the codepaths involved
behave sensibly), ...

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

... that would be the logical conclusion, I think.

As a longer term fix for better portability (i.e. system libraries
may not behave exactly the way how the codepaths and POSIX expects
wrt to the errors detected), it may become necessary to change the
function to yield "how the call failed" information in addition to
"here is the block of bytes I read for the object", without relying
on particular errno.  But as a shorter term solution, ...

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

... I think this is a reasonable thing to do regardless.

Thanks.

Re: ERANGE strikes again on my Windows build; RFH

[Jonathan Nieder]

Hi,

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.

Thanks and hope that helps,
Jonathan

Re: ERANGE strikes again on my Windows build; RFH

[Johannes Sixt]

Am 30.12.19 um 19:06 schrieb Jonathan Nieder:
> 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.

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

Re: ERANGE strikes again on my Windows build; RFH

[Jonathan Nieder]

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.

[...]
>> 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:

Thanks, will try it out.

Re: ERANGE strikes again on my Windows build; RFH

[Michal Suchánek]

On Mon, Dec 30, 2019 at 10:49:48AM -0800, Jonathan Nieder wrote:
> 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.

That works completely fine if the code in question also sets errno to 0
in case there is some other leftover value from a previous library call.

Thanks

Michal