* ERANGE strikes again on my Windows build; RFH @ 2019-12-28 15:41 Johannes Sixt 2019-12-29 14:29 ` Torsten Bögershausen ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Johannes Sixt @ 2019-12-28 15:41 UTC (permalink / raw) To: Git Mailing List 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-28 15:41 ERANGE strikes again on my Windows build; RFH Johannes Sixt @ 2019-12-29 14:29 ` Torsten Bögershausen 2019-12-29 14:43 ` Andreas Schwab 2019-12-29 17:25 ` Alban Gruin 2019-12-30 17:42 ` Junio C Hamano 2019-12-30 18:06 ` Jonathan Nieder 2 siblings, 2 replies; 10+ messages in thread From: Torsten Bögershausen @ 2019-12-29 14:29 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-29 14:29 ` Torsten Bögershausen @ 2019-12-29 14:43 ` Andreas Schwab 2019-12-29 17:25 ` Alban Gruin 1 sibling, 0 replies; 10+ messages in thread From: Andreas Schwab @ 2019-12-29 14:43 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Johannes Sixt, Git Mailing List 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." ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-29 14:29 ` Torsten Bögershausen 2019-12-29 14:43 ` Andreas Schwab @ 2019-12-29 17:25 ` Alban Gruin 2019-12-29 18:08 ` Johannes Sixt 1 sibling, 1 reply; 10+ messages in thread From: Alban Gruin @ 2019-12-29 17:25 UTC (permalink / raw) To: Torsten Bögershausen, Johannes Sixt; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-29 17:25 ` Alban Gruin @ 2019-12-29 18:08 ` Johannes Sixt 0 siblings, 0 replies; 10+ messages in thread From: Johannes Sixt @ 2019-12-29 18:08 UTC (permalink / raw) To: Alban Gruin, Torsten Bögershausen; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-28 15:41 ERANGE strikes again on my Windows build; RFH Johannes Sixt 2019-12-29 14:29 ` Torsten Bögershausen @ 2019-12-30 17:42 ` Junio C Hamano 2019-12-30 18:06 ` Jonathan Nieder 2 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2019-12-30 17:42 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-28 15:41 ERANGE strikes again on my Windows build; RFH Johannes Sixt 2019-12-29 14:29 ` Torsten Bögershausen 2019-12-30 17:42 ` Junio C Hamano @ 2019-12-30 18:06 ` Jonathan Nieder 2019-12-30 18:46 ` Johannes Sixt 2 siblings, 1 reply; 10+ messages in thread From: Jonathan Nieder @ 2019-12-30 18:06 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-30 18:06 ` Jonathan Nieder @ 2019-12-30 18:46 ` Johannes Sixt 2019-12-30 18:49 ` Jonathan Nieder 0 siblings, 1 reply; 10+ messages in thread From: Johannes Sixt @ 2019-12-30 18:46 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-30 18:46 ` Johannes Sixt @ 2019-12-30 18:49 ` Jonathan Nieder 2020-01-05 15:27 ` Michal Suchánek 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Nieder @ 2019-12-30 18:49 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: ERANGE strikes again on my Windows build; RFH 2019-12-30 18:49 ` Jonathan Nieder @ 2020-01-05 15:27 ` Michal Suchánek 0 siblings, 0 replies; 10+ messages in thread From: Michal Suchánek @ 2020-01-05 15:27 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Johannes Sixt, Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-05 15:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-28 15:41 ERANGE strikes again on my Windows build; RFH Johannes Sixt 2019-12-29 14:29 ` Torsten Bögershausen 2019-12-29 14:43 ` Andreas Schwab 2019-12-29 17:25 ` Alban Gruin 2019-12-29 18:08 ` Johannes Sixt 2019-12-30 17:42 ` Junio C Hamano 2019-12-30 18:06 ` Jonathan Nieder 2019-12-30 18:46 ` Johannes Sixt 2019-12-30 18:49 ` Jonathan Nieder 2020-01-05 15:27 ` Michal Suchánek
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).