{"thread":{"id":"52532","subject":"ERANGE strikes again on my Windows build; RFH","startedAt":"2019-12-28T15:41:48Z","lastAt":"2020-01-05T15:27:37Z","messageCount":10,"participants":["Johannes Sixt","Torsten Bögershausen","Andreas Schwab","Alban Gruin","Junio C Hamano","Jonathan Nieder","Michal Suchánek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389023","messageId":"6eb02a73-9dcb-f1fc-f015-80e71e9910d6@kdbg.org","threadId":"52532","inReplyTo":null,"subject":"ERANGE strikes again on my Windows build; RFH","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-28T15:41:42Z","receivedAt":"2019-12-28T15:41:48Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"In sha1-file.c:read_object_file_extended() we have the following pattern:\n\n\terrno = 0;\n\tdata = read_object(r, repl, type, size);\n\tif (data)\n\t\treturn data;\n\n\tif (errno && errno != ENOENT)\n\t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n\nThat is, it is expected that read_object() does not change the value of\nerrno in the non-error case. I find it intriguing that we expect a quite\nlarge call graph that is behind read_object() to behave this way.\n\nWhat if a subordinate callee starts doing\n\n\tif (some_syscall(...) < 0) {\n\t\tif (errno == EEXIST) {\n\t\t\t/* never mind, that's OK */\n\t\t\t...\n\t\t}\n\t}\n\nWould it be required to reset errno to its previous value in this\nfailure-is-not-an-error case?\n\nThe problem in my Windows build is that one of these subordinate\nsyscalls is vsnprintf() (as part of a strbuf_add variant, I presume),\nand it fails with ERANGE when the buffer is too short. Do I have to\nmodify the vsnprintf emulation to restore errno?\n"},{"id":"389029","messageId":"20191229142909.7bmjbrroboitvnzq@tb-raspi4","threadId":"52532","inReplyTo":"6eb02a73-9dcb-f1fc-f015-80e71e9910d6@kdbg.org","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2019-12-29T14:29:09Z","receivedAt":"2019-12-29T14:29:13Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Dec 28, 2019 at 04:41:42PM +0100, Johannes Sixt wrote:\n> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>\n> \terrno = 0;\n> \tdata = read_object(r, repl, type, size);\n> \tif (data)\n> \t\treturn data;\n>\n> \tif (errno && errno != ENOENT)\n> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>\n> That is, it is expected that read_object() does not change the value of\n> errno in the non-error case. I find it intriguing that we expect a quite\n> large call graph that is behind read_object() to behave this way.\n>\n> What if a subordinate callee starts doing\n>\n> \tif (some_syscall(...) < 0) {\n> \t\tif (errno == EEXIST) {\n> \t\t\t/* never mind, that's OK */\n> \t\t\t...\n> \t\t}\n> \t}\n>\n> Would it be required to reset errno to its previous value in this\n> failure-is-not-an-error case?\n>\n> The problem in my Windows build is that one of these subordinate\n> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),\n> and it fails with ERANGE when the buffer is too short. Do I have to\n> modify the vsnprintf emulation to restore errno?\n\nIf you ask me: I think so, yes.\nAt least the documentation about vsnprintf does not mention that errno is touched at all.\nThat is the man pages for Linux and Mac OS, or see here:\nhttps://linux.die.net/man/3/vsnprintf\n\nIt would make sense to analyze the complete callstack, I think.\nIs your problem reproducable ?\n\nChanging the function strbuf_vaddf() strbuf.c seems to be straight forward to me.\n"},{"id":"389030","messageId":"87mubb8a5p.fsf@igel.home","threadId":"52532","inReplyTo":"20191229142909.7bmjbrroboitvnzq@tb-raspi4","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2019-12-29T14:43:30Z","receivedAt":"2019-12-29T14:43:36Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Dez 29 2019, Torsten Bögershausen wrote:\n\n> At least the documentation about vsnprintf does not mention that errno is touched at all.\n> That is the man pages for Linux and Mac OS, or see here:\n> https://linux.die.net/man/3/vsnprintf\n\nRefer to the POSIX docs for vsnprintf, which documents its errno usage.\nIn general, any library call can touch errno, unless it is explicitly\ndocumented _not_ to touch it.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"389036","messageId":"b6e3fc5b-1d16-f7c9-0069-a2938b00f0fc@kdbg.org","threadId":"52532","inReplyTo":"a32e075b-9e6c-2b6a-8619-0330501eee97@gmail.com","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-29T18:08:14Z","receivedAt":"2019-12-29T18:08:21Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 29.12.19 um 18:25 schrieb Alban Gruin:\n> Le 29/12/2019 à 15:29, Torsten Bögershausen a écrit :\n>> On Sat, Dec 28, 2019 at 04:41:42PM +0100, Johannes Sixt wrote:\n>>> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>>>\n>>> \terrno = 0;\n>>> \tdata = read_object(r, repl, type, size);\n>>> \tif (data)\n>>> \t\treturn data;\n>>>\n>>> \tif (errno && errno != ENOENT)\n>>> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>>>\n>>> That is, it is expected that read_object() does not change the value of\n>>> errno in the non-error case. I find it intriguing that we expect a quite\n>>> large call graph that is behind read_object() to behave this way.\n>>>\n>>> What if a subordinate callee starts doing\n>>>\n>>> \tif (some_syscall(...) < 0) {\n>>> \t\tif (errno == EEXIST) {\n>>> \t\t\t/* never mind, that's OK */\n>>> \t\t\t...\n>>> \t\t}\n>>> \t}\n>>>\n>>> Would it be required to reset errno to its previous value in this\n>>> failure-is-not-an-error case?\n>>>\n>>> The problem in my Windows build is that one of these subordinate\n>>> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),\n>>> and it fails with ERANGE when the buffer is too short. Do I have to\n>>> modify the vsnprintf emulation to restore errno?\n>>\n>> If you ask me: I think so, yes.\n>> At least the documentation about vsnprintf does not mention that errno is touched at all.\n>> That is the man pages for Linux and Mac OS, or see here:\n>> https://linux.die.net/man/3/vsnprintf\n>>\n>> It would make sense to analyze the complete callstack, I think.\n>> Is your problem reproducable ?\n>>\n>> Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.\n>>\n> \n> According to the standard, vsnprintf() _can_ change errno[1] (and the\n> BSDs do so[2][3][4].)  But apparently, not to ERANGE.\n\nI am not worried about errno being set (or to what value) when there\nactually is an error. I am asking what to do when there is actually *no*\nerror. In my vsnprintf emulation, the case where ERANGE happens is *not*\nan error as far as the emulation is concerned.\n\nWhat if in the huge call graph behind read_object() some function\nchanges errno to, say, EEXIST, EISDIR, or ENODIR and the condition under\nwhich this happens is *not* an error in that context? Is the function\nrequired to restore the original errno?\n\nConsider the task to create file \"foo/bar.c\". We would have to\nmkdir(\"foo\"), but it is *not* an error when mkdir() fails with errno ==\nEEXIST. Are we required to reset errno back to its old value?\n\n(I know, read_object() is unlikely to allocate files, but I think I have\nto explain in some way that the context may define that there is no\nerror -- even though a lower-level function failed and modified errno.)\n\n-- Hannes\n"},{"id":"389037","messageId":"a32e075b-9e6c-2b6a-8619-0330501eee97@gmail.com","threadId":"52532","inReplyTo":"20191229142909.7bmjbrroboitvnzq@tb-raspi4","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-12-29T17:25:28Z","receivedAt":"2019-12-29T18:15:27Z","isPatch":false,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi,\n\nLe 29/12/2019 à 15:29, Torsten Bögershausen a écrit :\n> On Sat, Dec 28, 2019 at 04:41:42PM +0100, Johannes Sixt wrote:\n>> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>>\n>> \terrno = 0;\n>> \tdata = read_object(r, repl, type, size);\n>> \tif (data)\n>> \t\treturn data;\n>>\n>> \tif (errno && errno != ENOENT)\n>> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>>\n>> That is, it is expected that read_object() does not change the value of\n>> errno in the non-error case. I find it intriguing that we expect a quite\n>> large call graph that is behind read_object() to behave this way.\n>>\n>> What if a subordinate callee starts doing\n>>\n>> \tif (some_syscall(...) < 0) {\n>> \t\tif (errno == EEXIST) {\n>> \t\t\t/* never mind, that's OK */\n>> \t\t\t...\n>> \t\t}\n>> \t}\n>>\n>> Would it be required to reset errno to its previous value in this\n>> failure-is-not-an-error case?\n>>\n>> The problem in my Windows build is that one of these subordinate\n>> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),\n>> and it fails with ERANGE when the buffer is too short. Do I have to\n>> modify the vsnprintf emulation to restore errno?\n> \n> If you ask me: I think so, yes.\n> At least the documentation about vsnprintf does not mention that errno is touched at all.\n> That is the man pages for Linux and Mac OS, or see here:\n> https://linux.die.net/man/3/vsnprintf\n> \n> It would make sense to analyze the complete callstack, I think.\n> Is your problem reproducable ?\n> \n> Changing the function strbuf_vaddf() strbuf.c seems to be straight forward to me.\n> \n\nAccording to the standard, vsnprintf() _can_ change errno[1] (and the\nBSDs do so[2][3][4].)  But apparently, not to ERANGE.\n\n[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/vfprintf.html\n[2]\nhttps://www.freebsd.org/cgi/man.cgi?query=vsnprintf&manpath=FreeBSD+12.1-RELEASE\n[3] https://www.freebsd.org/cgi/man.cgi?query=vsnprintf&manpath=NetBSD+8.1\n[4] https://man.openbsd.org/vsnprintf\n\nCheers,\nAlban\n\n"},{"id":"389082","messageId":"xmqqh81hhfqn.fsf@gitster-ct.c.googlers.com","threadId":"52532","inReplyTo":"6eb02a73-9dcb-f1fc-f015-80e71e9910d6@kdbg.org","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-30T17:42:40Z","receivedAt":"2019-12-30T17:42:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>\n> \terrno = 0;\n> \tdata = read_object(r, repl, type, size);\n> \tif (data)\n> \t\treturn data;\n>\n> \tif (errno && errno != ENOENT)\n> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>\n> That is, it is expected that read_object() does not change the value of\n> errno in the non-error case. I find it intriguing that we expect a quite\n> large call graph that is behind read_object() to behave this way.\n\nYeah, that is somewhat unfortunate and dubious, but as long as it\nworks (i.e. various system library calls the codepaths involved\nbehave sensibly), ...\n\n> What if a subordinate callee starts doing\n>\n> \tif (some_syscall(...) < 0) {\n> \t\tif (errno == EEXIST) {\n> \t\t\t/* never mind, that's OK */\n> \t\t\t...\n> \t\t}\n> \t}\n>\n> Would it be required to reset errno to its previous value in this\n> failure-is-not-an-error case?\n\n... that would be the logical conclusion, I think.\n\nAs a longer term fix for better portability (i.e. system libraries\nmay not behave exactly the way how the codepaths and POSIX expects\nwrt to the errors detected), it may become necessary to change the\nfunction to yield \"how the call failed\" information in addition to\n\"here is the block of bytes I read for the object\", without relying\non particular errno.  But as a shorter term solution, ...\n\n> The problem in my Windows build is that one of these subordinate\n> syscalls is vsnprintf() (as part of a strbuf_add variant, I presume),\n> and it fails with ERANGE when the buffer is too short. Do I have to\n> modify the vsnprintf emulation to restore errno?\n\n... I think this is a reasonable thing to do regardless.\n\nThanks.\n"},{"id":"389083","messageId":"20191230180653.GA57251@google.com","threadId":"52532","inReplyTo":"6eb02a73-9dcb-f1fc-f015-80e71e9910d6@kdbg.org","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-30T18:06:53Z","receivedAt":"2019-12-30T18:06:57Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Sixt wrote:\n\n> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>\n> \terrno = 0;\n> \tdata = read_object(r, repl, type, size);\n> \tif (data)\n> \t\treturn data;\n>\n> \tif (errno && errno != ENOENT)\n> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>\n> That is, it is expected that read_object() does not change the value of\n> errno in the non-error case. I find it intriguing that we expect a quite\n> large call graph that is behind read_object() to behave this way.\n\nYes, this seems dubious.\n\nIn fact this is only inspecting errno in the returned-NULL case.  If I\nlook only at the code above and not at the implementation of\nread_object, then I would say that the bug is the 'errno &&' part: when\nerrno is meaningful for a function for a given return value, the usual\nconvention is\n\n (1) it *always* sets errno for errors, not conditionally\n (2) it never sets errno to 0\n\nThere are some exceptions (like strtoul) but they are few and\nunfortunate, not something to be imitated.\n\nDo you have more details about the case where read_object is expected\nto produce errno == 0?  I'm wondering whether we forgot to set 'errno\n= ENOENT' explicitly somewhere.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"389088","messageId":"54522fee-0796-df46-a3cf-4331537ecf59@kdbg.org","threadId":"52532","inReplyTo":"20191230180653.GA57251@google.com","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-30T18:46:13Z","receivedAt":"2019-12-30T18:46:19Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.12.19 um 19:06 schrieb Jonathan Nieder:\n> Johannes Sixt wrote:\n> \n>> In sha1-file.c:read_object_file_extended() we have the following pattern:\n>>\n>> \terrno = 0;\n>> \tdata = read_object(r, repl, type, size);\n>> \tif (data)\n>> \t\treturn data;\n>>\n>> \tif (errno && errno != ENOENT)\n>> \t\tdie_errno(_(\"failed to read object %s\"), oid_to_hex(oid));\n>>\n>> That is, it is expected that read_object() does not change the value of\n>> errno in the non-error case. I find it intriguing that we expect a quite\n>> large call graph that is behind read_object() to behave this way.\n> \n> Yes, this seems dubious.\n> \n> In fact this is only inspecting errno in the returned-NULL case.  If I\n> look only at the code above and not at the implementation of\n> read_object, then I would say that the bug is the 'errno &&' part: when\n> errno is meaningful for a function for a given return value, the usual\n> convention is\n> \n>  (1) it *always* sets errno for errors, not conditionally\n\nYou seem to understand that errno isn't set somewhere where it should be\nset. But the problem is not absence of setting errno, but abundance of\nsetting errno; in particular, when functions receive errors from\nlower-level functions, but then indicate to the higher levels that\neverything's fine; then the higher levels observe errno when they shouldn't.\n\n>  (2) it never sets errno to 0\n> \n> There are some exceptions (like strtoul) but they are few and\n> unfortunate, not something to be imitated.\n> \n> Do you have more details about the case where read_object is expected\n> to produce errno == 0?  I'm wondering whether we forgot to set 'errno\n> = ENOENT' explicitly somewhere.\n\nI don't think that forgetting to set ENOENT is the problem.\n\nIt happens reproducibly in test 5 of t0410-partial-clone:\n\n    ++ git -C repo fsck\n    Checking object directories: 100% (256/256), done.\n    Checking objects: 100% (1/1), done.\n    fatal: failed to read object\n383670739c2f993999f3c10911ac5cb5c6591523: Result too large\n\nwhen it should be\n\n    Checking object directories: 100% (256/256), done.\n    Checking objects: 100% (1/1), done.\n    dangling tag e5f4cb9fd329c512b08fb81a8e6b1f5e27658263\n\n-- Hannes\n"},{"id":"389089","messageId":"20191230184948.GC57251@google.com","threadId":"52532","inReplyTo":"54522fee-0796-df46-a3cf-4331537ecf59@kdbg.org","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-30T18:49:48Z","receivedAt":"2019-12-30T18:49:52Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Sixt wrote:\n> Am 30.12.19 um 19:06 schrieb Jonathan Nieder:\n\n>>                                                                    when\n>> errno is meaningful for a function for a given return value, the usual\n>> convention is\n>>\n>>  (1) it *always* sets errno for errors, not conditionally\n>\n> You seem to understand that errno isn't set somewhere where it should be\n> set.\n\nOn the contrary: this caller is using errno as an error *indicator*\ninstead of a way of *distinguishing* between errors (or to put it\nanother way, this caller is treating `errno == 0` as a meaningful\ncondition).  This means the calling code is buggy.\n\n[...]\n>> Do you have more details about the case where read_object is expected\n>> to produce errno == 0?  I'm wondering whether we forgot to set 'errno\n>> = ENOENT' explicitly somewhere.\n>\n> I don't think that forgetting to set ENOENT is the problem.\n>\n> It happens reproducibly in test 5 of t0410-partial-clone:\n\nThanks, will try it out.\n"},{"id":"389251","messageId":"20200105152732.GR4113@kitsune.suse.cz","threadId":"52532","inReplyTo":"20191230184948.GC57251@google.com","subject":"Re: ERANGE strikes again on my Windows build; RFH","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2020-01-05T15:27:32Z","receivedAt":"2020-01-05T15:27:37Z","isPatch":false,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Mon, Dec 30, 2019 at 10:49:48AM -0800, Jonathan Nieder wrote:\n> Johannes Sixt wrote:\n> > Am 30.12.19 um 19:06 schrieb Jonathan Nieder:\n> \n> >>                                                                    when\n> >> errno is meaningful for a function for a given return value, the usual\n> >> convention is\n> >>\n> >>  (1) it *always* sets errno for errors, not conditionally\n> >\n> > You seem to understand that errno isn't set somewhere where it should be\n> > set.\n> \n> On the contrary: this caller is using errno as an error *indicator*\n> instead of a way of *distinguishing* between errors (or to put it\n> another way, this caller is treating `errno == 0` as a meaningful\n> condition).  This means the calling code is buggy.\n\nThat works completely fine if the code in question also sets errno to 0\nin case there is some other leftover value from a previous library call.\n\nThanks\n\nMichal\n"}]}