{"thread":{"id":"46754","subject":"Git 2.14.1: t6500: error during test on musl libc","startedAt":"2017-09-15T02:50:03Z","lastAt":"2017-09-17T03:38:11Z","messageCount":11,"participants":["A. Wilcox","Kevin Daudt","Jeff King","Rich Felker","Junio C Hamano","Szabolcs Nagy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"328095","messageId":"59BB3E40.7020804@adelielinux.org","threadId":"46754","inReplyTo":null,"subject":"Git 2.14.1: t6500: error during test on musl libc","fromName":"A. Wilcox","fromEmail":"awilfox@adelielinux.org","sentAt":"2017-09-15T02:43:12Z","receivedAt":"2017-09-15T02:50:03Z","isPatch":false,"sender":{"key":"awilfox@adelielinux.org","avatar":"https://gravatar.com/avatar/7dfeb1949d7201fcf186c1ef0d0d0664f667c03834d1630eb7d057e7d37c2266?d=mp&s=160"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA256\n\nHi there,\n\nWhile bumping Git's version for our Linux distribution to 2.14.1, I've\nrun in to a new test failure in t6500-gc.sh. This is the output of\nthe failing test with debug=t verbose=t:\n\n\nexpecting success:\n # make sure we run a background auto-gc\n test_commit make-pack &&\n git repack &&\n test_config gc.autopacklimit 1 &&\n test_config gc.autodetach true &&\n\n # create a ref whose loose presence we can use to detect a\npack-refs run\n git update-ref refs/heads/should-be-loose HEAD &&\n test_path_is_file .git/refs/heads/should-be-loose &&\n\n # now fake a concurrent gc that holds the lock; we can use our\n # shell pid so that it looks valid.\n hostname=$(hostname || echo unknown) &&\n printf \"$$ %s\" \"$hostname\" >.git/gc.pid &&\n\n # our gc should exit zero without doing anything\n run_and_wait_for_auto_gc &&\n test_path_is_file .git/refs/heads/should-be-loose\n\n[master 28ecdda] make-pack\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 make-pack.t\nCounting objects: 3, done.\nDelta compression using up to 8 threads.\nCompressing objects: 100% (2/2), done.\nWriting objects: 100% (3/3), done.\nTotal 3 (delta 0), reused 0 (delta 0)\nAuto packing the repository in background for optimum performance.\nSee \"git help gc\" for manual housekeeping.\nFile .git/refs/heads/should-be-loose doesn't exist.\nnot ok 8 - background auto gc respects lock for all operations\n#\n# # make sure we run a background auto-gc\n# test_commit make-pack &&\n# git repack &&\n# test_config gc.autopacklimit 1 &&\n# test_config gc.autodetach true &&\n#\n# # create a ref whose loose presence we can use to\ndetect a pack-refs run\n# git update-ref refs/heads/should-be-loose HEAD &&\n# test_path_is_file .git/refs/heads/should-be-loose &&\n#\n# # now fake a concurrent gc that holds the lock; we can\nuse our\n# # shell pid so that it looks valid.\n# hostname=$(hostname || echo unknown) &&\n# printf \"$$ %s\" \"$hostname\" >.git/gc.pid &&\n#\n# # our gc should exit zero without doing anything\n# run_and_wait_for_auto_gc &&\n# test_path_is_file .git/refs/heads/should-be-loose\n#\n\n# failed 1 among 8 test(s)\n1..8\n\n\nI admit I am mostly blind with the Git gc system. Should I use strace\non the git-gc process at the end? How would I accomplish that? Is\nthere a better way of debugging this error further?\n\nCore system stats:\n\nIntel x86_64 E3-1280 v3 @ 3.60 GHz\nmusl libc 1.1.16+20\ngit 2.14.1, vanilla except for a patch to an earlier test due to\nmusl's inability to cope with EUC-JP\nbash 4.3.48(1)-release\n\nThank you very much.\n\nAll the best,\n- --arw\n\n- -- \nA. Wilcox (awilfox)\nProject Lead, Adélie Linux\nhttp://adelielinux.org\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v2\n\niQIcBAEBCAAGBQJZuz4yAAoJEMspy1GSK50UORwP/0Jxfp3xzexh27tSJlXYWS/g\ng9QK8Xmid+3A0R696Vb2GguKg2roCcTmM2anR7iD1B2f2W31sgf+8M5mnJRHyJ1p\ngeEeqwrTdpCk6jQ/1Pj03L0NOftb1ftR6hcoVujBFAOph4jRlRdZDPA87fe6snrh\nq99C3LoDXQcyK6WWJwzX+t2wOplKgpGJP8wTAaZ0AHoUwVS5CLPl8tP2XaY4kLfD\nZPPcvtp9wisVzzZ2ssE/CLGd38EbenNNZ6OJCBFJIHmlwey4G2isZ9kk6fVIHXi2\nunBJ8yVqI7hQKmQFSVQMMSFSd9azhHnDjTBO5mzWeRK9HNVMda3LZsXTtVeswnRs\nlN/ASMdt5KdfpNy/plFB7yDWLlQSQY7j1mxBMR8lL3AdVVQUbJppDM795tt+rn6a\nNCE2ESZMWd/QEULmT92AbkNJTj5ibBEoubnVTka05KMjaBLwIauhpqU5XxLFq2UH\ny3JYQU9hm0E7dQE0CLXxIm5/574T6bBUgp1cXH3CjxkeUYKR1USVKtDfBV6t/Qmt\nxlDZKPEfjKbTvL3KUF33G+eAp55wTwrJTaWlOp8A/JqooXavYghcsuFhYtCPJ8qo\nfFUa8kBZP70E/O7JkycUu8wi7p42+j1a8gR6/AnPG2u2wyoiosLCxHX+nll4gKmN\nb6BuiRn0Z9ie5xw4xcMR\n=Vf8Z\n-----END PGP SIGNATURE-----\n"},{"id":"328107","messageId":"20170915063740.GB21499@alpha.vpn.ikke.info","threadId":"46754","inReplyTo":"59BB3E40.7020804@adelielinux.org","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-09-15T06:37:40Z","receivedAt":"2017-09-15T06:54:27Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:\n> -----BEGIN PGP SIGNED MESSAGE-----\n> Hash: SHA256\n> \n> Hi there,\n> \n> While bumping Git's version for our Linux distribution to 2.14.1, I've\n> run in to a new test failure in t6500-gc.sh. This is the output of\n> the failing test with debug=t verbose=t:\n\nThis is a new test introduced by c45af94db \n(gc: run pre-detach operations under lock, 2017-07-11) which was\nincluded in v2.14.0.\n\nSo it might be that this was already a problem for a longer time, only\njust recently uncovered.\n\n> \n> \n> expecting success:\n> # make sure we run a background auto-gc\n> test_commit make-pack &&\n> git repack &&\n> test_config gc.autopacklimit 1 &&\n> test_config gc.autodetach true &&\n> \n> # create a ref whose loose presence we can use to detect a\n> pack-refs run\n> git update-ref refs/heads/should-be-loose HEAD &&\n> test_path_is_file .git/refs/heads/should-be-loose &&\n> \n> # now fake a concurrent gc that holds the lock; we can use our\n> # shell pid so that it looks valid.\n> hostname=$(hostname || echo unknown) &&\n> printf \"$$ %s\" \"$hostname\" >.git/gc.pid &&\n> \n> # our gc should exit zero without doing anything\n> run_and_wait_for_auto_gc &&\n> test_path_is_file .git/refs/heads/should-be-loose\n> \n> [master 28ecdda] make-pack\n> Author: A U Thor \n> 1 file changed, 1 insertion(+)\n> create mode 100644 make-pack.t\n> Counting objects: 3, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (2/2), done.\n> Writing objects: 100% (3/3), done.\n> Total 3 (delta 0), reused 0 (delta 0)\n> Auto packing the repository in background for optimum performance.\n> See \"git help gc\" for manual housekeeping.\n> File .git/refs/heads/should-be-loose doesn't exist.\n> not ok 8 - background auto gc respects lock for all operations\n> #\n> # # make sure we run a background auto-gc\n> # test_commit make-pack &&\n> # git repack &&\n> # test_config gc.autopacklimit 1 &&\n> # test_config gc.autodetach true &&\n> #\n> # # create a ref whose loose presence we can use to\n> detect a pack-refs run\n> # git update-ref refs/heads/should-be-loose HEAD &&\n> # test_path_is_file .git/refs/heads/should-be-loose &&\n> #\n> # # now fake a concurrent gc that holds the lock; we can\n> use our\n> # # shell pid so that it looks valid.\n> # hostname=$(hostname || echo unknown) &&\n> # printf \"$$ %s\" \"$hostname\" >.git/gc.pid &&\n> #\n> # # our gc should exit zero without doing anything\n> # run_and_wait_for_auto_gc &&\n> # test_path_is_file .git/refs/heads/should-be-loose\n> #\n> \n> # failed 1 among 8 test(s)\n> 1..8\n> \n> \n> I admit I am mostly blind with the Git gc system. Should I use strace\n> on the git-gc process at the end? How would I accomplish that? Is\n> there a better way of debugging this error further?\n> \n> Core system stats:\n> \n> Intel x86_64 E3-1280 v3 @ 3.60 GHz\n> musl libc 1.1.16+20\n> git 2.14.1, vanilla except for a patch to an earlier test due to\n> musl's inability to cope with EUC-JP\n> bash 4.3.48(1)-release\n> \n> Thank you very much.\n> \n> All the best,\n> - --arw\n> \n> - -- \n> A. Wilcox (awilfox)\n> Project Lead, Adélie Linux\n> http://adelielinux.org\n> -----BEGIN PGP SIGNATURE-----\n> Version: GnuPG v2\n> \n> iQIcBAEBCAAGBQJZuz4yAAoJEMspy1GSK50UORwP/0Jxfp3xzexh27tSJlXYWS/g\n> g9QK8Xmid+3A0R696Vb2GguKg2roCcTmM2anR7iD1B2f2W31sgf+8M5mnJRHyJ1p\n> geEeqwrTdpCk6jQ/1Pj03L0NOftb1ftR6hcoVujBFAOph4jRlRdZDPA87fe6snrh\n> q99C3LoDXQcyK6WWJwzX+t2wOplKgpGJP8wTAaZ0AHoUwVS5CLPl8tP2XaY4kLfD\n> ZPPcvtp9wisVzzZ2ssE/CLGd38EbenNNZ6OJCBFJIHmlwey4G2isZ9kk6fVIHXi2\n> unBJ8yVqI7hQKmQFSVQMMSFSd9azhHnDjTBO5mzWeRK9HNVMda3LZsXTtVeswnRs\n> lN/ASMdt5KdfpNy/plFB7yDWLlQSQY7j1mxBMR8lL3AdVVQUbJppDM795tt+rn6a\n> NCE2ESZMWd/QEULmT92AbkNJTj5ibBEoubnVTka05KMjaBLwIauhpqU5XxLFq2UH\n> y3JYQU9hm0E7dQE0CLXxIm5/574T6bBUgp1cXH3CjxkeUYKR1USVKtDfBV6t/Qmt\n> xlDZKPEfjKbTvL3KUF33G+eAp55wTwrJTaWlOp8A/JqooXavYghcsuFhYtCPJ8qo\n> fFUa8kBZP70E/O7JkycUu8wi7p42+j1a8gR6/AnPG2u2wyoiosLCxHX+nll4gKmN\n> b6BuiRn0Z9ie5xw4xcMR\n> =Vf8Z\n> -----END PGP SIGNATURE-----\n"},{"id":"328108","messageId":"20170915064324.GC21499@alpha.vpn.ikke.info","threadId":"46754","inReplyTo":"20170915063740.GB21499@alpha.vpn.ikke.info","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-09-15T06:43:24Z","receivedAt":"2017-09-15T07:00:09Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:\n> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:\n> > -----BEGIN PGP SIGNED MESSAGE-----\n> > Hash: SHA256\n> > \n> > Hi there,\n> > \n> > While bumping Git's version for our Linux distribution to 2.14.1, I've\n> > run in to a new test failure in t6500-gc.sh. This is the output of\n> > the failing test with debug=t verbose=t:\n> \n> This is a new test introduced by c45af94db \n> (gc: run pre-detach operations under lock, 2017-07-11) which was\n> included in v2.14.0.\n> \n> So it might be that this was already a problem for a longer time, only\n> just recently uncovered.\n> \n\nAdding Jeff King to CC\n"},{"id":"328118","messageId":"20170915113011.emko6q5utb7x4bvu@sigill.intra.peff.net","threadId":"46754","inReplyTo":"20170915063740.GB21499@alpha.vpn.ikke.info","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-15T11:30:11Z","receivedAt":"2017-09-15T11:30:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:\n\n> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:\n> > -----BEGIN PGP SIGNED MESSAGE-----\n> > Hash: SHA256\n> > \n> > Hi there,\n> > \n> > While bumping Git's version for our Linux distribution to 2.14.1, I've\n> > run in to a new test failure in t6500-gc.sh. This is the output of\n> > the failing test with debug=t verbose=t:\n> \n> This is a new test introduced by c45af94db \n> (gc: run pre-detach operations under lock, 2017-07-11) which was\n> included in v2.14.0.\n> \n> So it might be that this was already a problem for a longer time, only\n> just recently uncovered.\n\nThe code change there is not all that big. Mostly we're just checking\nthat the lock is actually respected. The lock code doesn't exercise libc\nall that much. It does use fscanf, which I guess is a little exotic for\nus. It's also possible that hostname() doesn't behave quite as we\nexpect.\n\nIf you instrument gc like the patch below, what does it report when you\nrun:\n\n GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8\n\nI get:\n\n [...]\n trace: built-in: git 'gc' '--auto'\n Auto packing the repository in background for optimum performance.\n See \"git help gc\" for manual housekeeping.\n debug: gc lock already held by $my_hostname\n [...]\n\nIf you get \"acquired gc lock\", then the problem is in\nlock_repo_for_gc(), and I'd suspect some problem with fscanf or\nhostname.\n\n-Peff\n\n---\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c22787ac72..a7450a0058 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -242,9 +242,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tint fd;\n \tchar *pidfile_path;\n \n-\tif (is_tempfile_active(pidfile))\n+\tif (is_tempfile_active(pidfile)) {\n \t\t/* already locked */\n+\t\ttrace_printf(\"debug: we already hold the gc lock\");\n \t\treturn NULL;\n+\t}\n \n \tif (xgethostname(my_host, sizeof(my_host)))\n \t\txsnprintf(my_host, sizeof(my_host), \"unknown\");\n@@ -284,6 +286,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\t\trollback_lock_file(&lock);\n \t\t\t*ret_pid = pid;\n \t\t\tfree(pidfile_path);\n+\t\t\ttrace_printf(\"debug: gc lock already held by %s\", locking_host);\n \t\t\treturn locking_host;\n \t\t}\n \t}\n@@ -295,6 +298,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tcommit_lock_file(&lock);\n \tpidfile = register_tempfile(pidfile_path);\n \tfree(pidfile_path);\n+\ttrace_printf(\"debug: we have acquired the gc lock\");\n \treturn NULL;\n }\n \n"},{"id":"328181","messageId":"59BCAF81.3090206@adelielinux.org","threadId":"46754","inReplyTo":"20170915113011.emko6q5utb7x4bvu@sigill.intra.peff.net","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"A. Wilcox","fromEmail":"awilfox@adelielinux.org","sentAt":"2017-09-16T04:58:41Z","receivedAt":"2017-09-16T04:58:58Z","isPatch":false,"sender":{"key":"awilfox@adelielinux.org","avatar":"https://gravatar.com/avatar/7dfeb1949d7201fcf186c1ef0d0d0664f667c03834d1630eb7d057e7d37c2266?d=mp&s=160"},"body":"On 15/09/17 06:30, Jeff King wrote:\n> On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:\n> \n>> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:\n>>> -----BEGIN PGP SIGNED MESSAGE-----\n>>> Hash: SHA256\n>>>\n>>> Hi there,\n>>>\n>>> While bumping Git's version for our Linux distribution to 2.14.1, I've\n>>> run in to a new test failure in t6500-gc.sh. This is the output of\n>>> the failing test with debug=t verbose=t:\n>>\n>> This is a new test introduced by c45af94db \n>> (gc: run pre-detach operations under lock, 2017-07-11) which was\n>> included in v2.14.0.\n>>\n>> So it might be that this was already a problem for a longer time, only\n>> just recently uncovered.\n> \n> The code change there is not all that big. Mostly we're just checking\n> that the lock is actually respected. The lock code doesn't exercise libc\n> all that much. It does use fscanf, which I guess is a little exotic for\n> us. It's also possible that hostname() doesn't behave quite as we\n> expect.\n> \n> If you instrument gc like the patch below, what does it report when you\n> run:\n> \n> GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8\n> \n> I get:\n> \n> [...]\n> trace: built-in: git 'gc' '--auto'\n> Auto packing the repository in background for optimum performance.\n> See \"git help gc\" for manual housekeeping.\n> debug: gc lock already held by $my_hostname\n> [...]\n> \n> If you get \"acquired gc lock\", then the problem is in\n> lock_repo_for_gc(), and I'd suspect some problem with fscanf or\n> hostname.\n> \n> -Peff\n\n\nHey there Peff,\n\nWhat a corner-y corner case we have here. I believe the actual error is\nin the POSIX standard itself[1], as it is not clear what happens when\nthere are not enough characters to 'fill' the width specified with %c in\nfscanf:\n\n> c\n> Matches a sequence of bytes of the number specified by the field\n> width (1 if no field width is present in the conversion\n> specification).\n\nI've tested a number of machines:\n\n* OpenBSD 5.7/amd64\n* NetBSD 7.0/i386\n* FreeBSD 12/PowerPC\n* glibc/arm\n* Windows 7 with Microsoft Visual C++ 2013\n\nAll of them will allow a so-called \"short read\" and give you as many\ncharacters as they can, treating the phrase \"a sequence of bytes of the\nnumber specified\" as meaning \"*up to* the number\".\n\nThe musl libc treats this phrase as meaning \"*exactly* the number\", and\nfails if it cannot give you exactly the number you ask.\n\nIBM z/OS explicitly states in their documentation[2]:\n\n> Sequence of one or more characters as specified by field width\n\nAnd Microsoft similarly states[3]:\n\n> The width field is a positive decimal integer controlling the maximum\n> number of characters to be read for that field. No more than width\n> characters are converted and stored at the corresponding argument.\n> Fewer than width characters may be read if a whitespace character\n> (space, tab, or newline) or a character that cannot be converted\n> according to the given format occurs before width is reached.\n\nWhile musl's reading is correct from an English grammar point of view,\nit does not seem to be how any other implementation has read the standard.\n\n\nHowever! It gets better.\n\nThe ISO C standard, committee draft version April 12, 2011, states[4]:\n\n> c Matches a sequence of characters of exactly the number specified\n> by the field width (1 if no field width is present in the directive).\n\n\nSince \"[t]his volume of POSIX.1-2008 defers to the ISO C standard\", it\nstands to reason that this is the intended meaning and behaviour. Thus\nmeaning that literally every implementation, with the exception of the\nmusl libc, is breaking the ISO C standard.\n\n\nSince Git is specifically attempting to read in a host name, there may\nbe a solution: while 'c' guarantees that any byte will be read, and 's'\nwill skip whitespace, RFCs 952 and 1123 §2.1[5] specify that a network\nhost name must never contain whitespace. IDNA2008 §2.3.2.1[6] (and\nIDNA2003 before it) specifically removes ASCII whitespace characters\nfrom the valid set of Unicode codepoints for an IDNA host name[7].\nAdditionally, the buffer `locking_host` is already defined as an array\nof char of size HOST_NAME_MAX + 1, and the width specifier in fscanf is\nspecified as HOST_NAME_MAX. Therefore, it should be safe to change git\nto use the 's' type character. Additionally, I can confirm that this\nchange (patch attached) allows the Git test suite to pass on musl.\n\n\nI hope this message is informative. This was an exhausting, but\nnecessary, exercise in trying to ensure code correctness.\n\nI am cc'ing the musl list so that this information may live there as\nwell, in case someone in the future has issues with the 'c' type\nspecifier with fscanf on musl.\n\n\nAll the best,\n--arw\n\n\n[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html\n[2]:\nhttps://www.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.bpxbd00/fscanf.htm\n[3]: https://msdn.microsoft.com/en-us/library/xdb9w69d.aspx\n[4]: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf\n[5]: https://tools.ietf.org/html/rfc1123#section-2.1\n[6]: https://tools.ietf.org/html/rfc5890#section-2.3.2.1\n[7]: http://unicode.org/faq/idn.html#33\n-- \nA. Wilcox (awilfox)\nProject Lead, Adélie Linux\nhttp://adelielinux.org\n\n\nFrom afceb0f7755a87d0dd2194e95f26c9dc8f4bc688 Mon Sep 17 00:00:00 2001\nFrom: \"A. Wilcox\" \nDate: Fri, 15 Sep 2017 23:55:57 -0500\nSubject: [PATCH] gc: use 's' type character for fscanf\n\nThe ISO C standard states that using a field width together with the 'c'\ntype character will read the exact amount specified; if that amount of\nbytes is not available, a match error occurs.\n\nThis patch allows the t6500 test to pass on the musl libc, and `git gc`\nto behave correctly on syystems utilising musl.\n\nSigned-off-by: A. Wilcox \n---\n builtin/gc.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3c78fcb..bb2d6c1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -258,7 +258,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\tint should_exit;\n \n \t\tif (!scan_fmt)\n-\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX, HOST_NAME_MAX);\n+\t\t\tscan_fmt = xstrfmt(\"%s %%%ds\", \"%\"SCNuMAX, HOST_NAME_MAX);\n \t\tfp = fopen(pidfile_path, \"r\");\n \t\tmemset(locking_host, 0, sizeof(locking_host));\n \t\tshould_exit =\n-- \n2.10.0\n\n"},{"id":"328235","messageId":"20170916161322.GX1627@brightrain.aerifal.cx","threadId":"46754","inReplyTo":"59BCAF81.3090206@adelielinux.org","subject":"Re: [musl] Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Rich Felker","fromEmail":"dalias@libc.org","sentAt":"2017-09-16T16:13:22Z","receivedAt":"2017-09-16T16:24:12Z","isPatch":false,"sender":{"key":"dalias@libc.org","avatar":null},"body":"On Fri, Sep 15, 2017 at 11:58:41PM -0500, A. Wilcox wrote:\n> On 15/09/17 06:30, Jeff King wrote:\n> > On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:\n> > \n> >> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:\n> >>> -----BEGIN PGP SIGNED MESSAGE-----\n> >>> Hash: SHA256\n> >>>\n> >>> Hi there,\n> >>>\n> >>> While bumping Git's version for our Linux distribution to 2.14.1, I've\n> >>> run in to a new test failure in t6500-gc.sh. This is the output of\n> >>> the failing test with debug=t verbose=t:\n> >>\n> >> This is a new test introduced by c45af94db \n> >> (gc: run pre-detach operations under lock, 2017-07-11) which was\n> >> included in v2.14.0.\n> >>\n> >> So it might be that this was already a problem for a longer time, only\n> >> just recently uncovered.\n> > \n> > The code change there is not all that big. Mostly we're just checking\n> > that the lock is actually respected. The lock code doesn't exercise libc\n> > all that much. It does use fscanf, which I guess is a little exotic for\n> > us. It's also possible that hostname() doesn't behave quite as we\n> > expect.\n> > \n> > If you instrument gc like the patch below, what does it report when you\n> > run:\n> > \n> > GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8\n> > \n> > I get:\n> > \n> > [...]\n> > trace: built-in: git 'gc' '--auto'\n> > Auto packing the repository in background for optimum performance.\n> > See \"git help gc\" for manual housekeeping.\n> > debug: gc lock already held by $my_hostname\n> > [...]\n> > \n> > If you get \"acquired gc lock\", then the problem is in\n> > lock_repo_for_gc(), and I'd suspect some problem with fscanf or\n> > hostname.\n> > \n> > -Peff\n> \n> \n> Hey there Peff,\n> \n> What a corner-y corner case we have here. I believe the actual error is\n> in the POSIX standard itself[1], as it is not clear what happens when\n> there are not enough characters to 'fill' the width specified with %c in\n> fscanf:\n\nISO C specifies very clearly what happens, in 7.21.6.2 The fscanf\nfunction, paragraph 12: \n\n\tc\n\t\tMatches a sequence of characters of exactly the number\n\t\tspecified by the field width...\n\nNote the word \"exactly\". Thus a read of fewer characters is not a\nmatch.\n\nThere is an open glibc bug for this with classic Drepper behavior\nuntil his departure, followed by acknowledgement of the bug, but no\nfurther action I'm aware of:\n\nhttps://sourceware.org/bugzilla/show_bug.cgi?id=12701\n\nAny applications depending on the buggy glibc behavior should be\nfixed.\n\nRich\n"},{"id":"328236","messageId":"xmqqpoaqupo5.fsf@gitster.mtv.corp.google.com","threadId":"46754","inReplyTo":"59BCAF81.3090206@adelielinux.org","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-17T00:36:26Z","receivedAt":"2017-09-17T00:36:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"A. Wilcox\" writes:\n\n> While musl's reading is correct from an English grammar point of view,\n> it does not seem to be how any other implementation has read the standard.\n>\n> However! It gets better.\n>\n> The ISO C standard, committee draft version April 12, 2011, states[4]:\n>\n>> c Matches a sequence of characters of exactly the number specified\n>> by the field width (1 if no field width is present in the directive).\n> ...\n> Since Git is specifically attempting to read in a host name, there may\n> be a solution: while 'c' guarantees that any byte will be read, and 's'\n> will skip whitespace, RFCs 952 and 1123 §2.1[5] specify that a network\n> host name must never contain whitespace. IDNA2008 §2.3.2.1[6] (and\n> IDNA2003 before it) specifically removes ASCII whitespace characters\n> from the valid set of Unicode codepoints for an IDNA host name[7].\n> Additionally, the buffer `locking_host` is already defined as an array\n> of char of size HOST_NAME_MAX + 1, and the width specifier in fscanf is\n> specified as HOST_NAME_MAX. Therefore, it should be safe to change git\n> to use the 's' type character. Additionally, I can confirm that this\n> change (patch attached) allows the Git test suite to pass on musl.\n\nI did a quick scan for substring \"scanf\" and read through the\noutput, and it seems that this is the only one that wants to do the\nthis many characters, e.g. \"%42c\", conversion.\n\nI am a bit worried about the correctness of your conclusion, though.\n\nAs long as we are reading from the file written by us, because the\nstring we write as the hostname part comes from what we prepare in\nmy_host[HOST_NAME_MAX+1] using xgethostname(), we may know it would\nfit in locking_host[HOST_NAME_MAX+1]. But because HOST_NAME_MAX on\nmy platform may be shorter than what your platform uses, I'll run\nover the end of my buffer if I am reading the lockfile you write to\nnotice that the repository is in use from your host. After all, the\nreason why we write hostname in the file is because we expect the\nfilesystem is shared across different hosts, so relying on HOST_NAME_MAX\nto be the same across platforms would not be a good way to go.\n\nSo it seems to me that a real fix has to read the file ourselves and\nparse up to our HOST_NAME_MAX+1 to see if the hostname refers to us,\nand fscanf that cannot take \"slurp up to this many bytes\" is not\nuseful tool to implementing that parsing.\n\nThe current scan_fmt variable comes from da25bdb7 (\"use\nHOST_NAME_MAX to size buffers for gethostname(2)\", 2017-04-18), and\nbefore that, we used to use \"%\"SCNuMAX\" %127c\", which was already\nproblematic. The \"%127c\" part came from the very original of this\ncodepath in 64a99eb4 (\"gc: reject if another gc is running, unless\n--force is given\", 2013-08-08), whose first appearance in released\nversions was in v1.8.5, it seems. IOW, nobody tried to run Git with\nmusl C in the past 4 years and you are the first one to notice?\n\nThanks.\n"},{"id":"328239","messageId":"20170917011731.GD15263@port70.net","threadId":"46754","inReplyTo":"xmqqpoaqupo5.fsf@gitster.mtv.corp.google.com","subject":"Re: [musl] Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Szabolcs Nagy","fromEmail":"nsz@port70.net","sentAt":"2017-09-17T01:17:32Z","receivedAt":"2017-09-17T01:27:26Z","isPatch":false,"sender":{"key":"nsz@port70.net","avatar":null},"body":"* Junio C Hamano [2017-09-17 09:36:26 +0900]:\n> versions was in v1.8.5, it seems. IOW, nobody tried to run Git with\n> musl C in the past 4 years and you are the first one to notice?\n\ngit works fine on musl in practice, i use it for more than 4years now.\n\n"},{"id":"328241","messageId":"59BDD6AF.5090604@adelielinux.org","threadId":"46754","inReplyTo":"xmqqpoaqupo5.fsf@gitster.mtv.corp.google.com","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"A. Wilcox","fromEmail":"awilfox@adelielinux.org","sentAt":"2017-09-17T01:58:07Z","receivedAt":"2017-09-17T01:58:19Z","isPatch":false,"sender":{"key":"awilfox@adelielinux.org","avatar":"https://gravatar.com/avatar/7dfeb1949d7201fcf186c1ef0d0d0664f667c03834d1630eb7d057e7d37c2266?d=mp&s=160"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA256\n\nOn 16/09/17 19:36, Junio C Hamano wrote:\n> \"A. Wilcox\" writes:\n> \n>> While musl's reading is correct from an English grammar point of\n>> view, it does not seem to be how any other implementation has\n>> read the standard.\n>> \n>> However! It gets better.\n>> \n>> The ISO C standard, committee draft version April 12, 2011,\n>> states[4]:\n>> \n>>> c Matches a sequence of characters of exactly the number\n>>> specified by the field width (1 if no field width is present in\n>>> the directive).\n>> ... Since Git is specifically attempting to read in a host name,\n>> there may be a solution: while 'c' guarantees that any byte will\n>> be read, and 's' will skip whitespace, RFCs 952 and 1123 §2.1[5]\n>> specify that a network host name must never contain whitespace.\n>> IDNA2008 §2.3.2.1[6] (and IDNA2003 before it) specifically\n>> removes ASCII whitespace characters from the valid set of Unicode\n>> codepoints for an IDNA host name[7]. Additionally, the buffer\n>> `locking_host` is already defined as an array of char of size\n>> HOST_NAME_MAX + 1, and the width specifier in fscanf is specified\n>> as HOST_NAME_MAX. Therefore, it should be safe to change git to\n>> use the 's' type character. Additionally, I can confirm that\n>> this change (patch attached) allows the Git test suite to pass on\n>> musl.\n> \n> I did a quick scan for substring \"scanf\" and read through the \n> output, and it seems that this is the only one that wants to do\n> the this many characters, e.g. \"%42c\", conversion.\n> \n> I am a bit worried about the correctness of your conclusion,\n> though.\n\n\n%[width]s means read up to [width] non-ws characters. It is exactly\nwhat Git wants out of %[width]c, with the difference that 's' will\nstop at whitespace. Since hostnames cannot legally contain\nwhitespace, the difference is negligible.\n\n\n> As long as we are reading from the file written by us, because the \n> string we write as the hostname part comes from what we prepare in \n> my_host[HOST_NAME_MAX+1] using xgethostname(), we may know it\n> would fit in locking_host[HOST_NAME_MAX+1]. But because\n> HOST_NAME_MAX on my platform may be shorter than what your platform\n> uses, I'll run over the end of my buffer if I am reading the\n> lockfile you write to notice that the repository is in use from\n> your host. After all, the reason why we write hostname in the file\n> is because we expect the filesystem is shared across different\n> hosts, so relying on HOST_NAME_MAX to be the same across platforms\n> would not be a good way to go.\n\n\nYou cannot run over the buffer in any scenario: if the hostname is\nlonger than [width], then [width] + \\0 will be written to the buffer.\n Since the buffer is HOST_NAME_MAX+1 and the width is HOST_NAME_MAX,\nit stands to reason that you cannot overflow the buffer.\n\n\n> So it seems to me that a real fix has to read the file ourselves\n> and parse up to our HOST_NAME_MAX+1 to see if the hostname refers\n> to us, and fscanf that cannot take \"slurp up to this many bytes\" is\n> not useful tool to implementing that parsing.\n\n\nExcept that is *exactly* *what* *s* *does* (quoting C11 §7.21.6.2):\n\n9 An input item is defined as the longest sequence of input\n characters which does not exceed any specified field width\n\n\ns Matches a sequence of non-white-space characters.\n\n\n\nawilcox on elaine ~ $ cat -> myhost\nelaine.foxkit.us\nawilcox on elaine ~ $ cat -> test.c\n#include \nint main(void) {\n char test[9];\n fscanf(fopen(\"myhost\", \"r\"), \"%8s\", test);\n printf(\"result: %s\\n\", test);\n return 0;\n}\nawilcox on elaine ~ $ c99 -o test test.c\nawilcox on elaine ~ $ ./test\nresult: elaine.f\nawilcox on elaine ~ $ /usr/lib/libc.so\nmusl libc (powerpc)\nVersion 1.1.16\n\n\n> The current scan_fmt variable comes from da25bdb7 (\"use \n> HOST_NAME_MAX to size buffers for gethostname(2)\", 2017-04-18),\n> and before that, we used to use \"%\"SCNuMAX\" %127c\", which was\n> already problematic. The \"%127c\" part came from the very original\n> of this codepath in 64a99eb4 (\"gc: reject if another gc is running,\n> unless --force is given\", 2013-08-08), whose first appearance in\n> released versions was in v1.8.5, it seems. IOW, nobody tried to\n> run Git with musl C in the past 4 years and you are the first one\n> to notice?\n\n\nYes. Yes I am.\n\nBecause this is the first version that has a *test* that *tests* this\nbehaviour.\n\nThe odds that someone:\n\n* is using musl\n* with a Git repository over a network file system\n* with another platform with a different HOST_NAME_MAX\n* both concurrently running `git gc`, or the non-musl one being killed\n* with loose refs that they, for some reason, would notice not being\ngc'd properly\n\nis almost zero. However, it is theoretically something that could\nhappen, and that is the point of a test suite: testing code paths that\nare rarely used to ensure they are correct when needed. And this test\nhas proven that this code is *not* correct and will *not* function as\nintended when it is needed.\n\nThe fact that this code \"hasn't caused issues until now\" does not mean\nthat it is correct; it only means that you are lucky enough that\nnobody has attempted to use it until now.\n\nLike it or not, the ISO C standard states exactly how fscanf should\nbehave, and you are relying on non-standard behaviour for a *lock* file.\n\nIf you are uninterested in conforming to the standard of the language\nin which the source control system is written in (C), then perhaps it\nshould be written in a different language. Perhaps Perl, or Rust.\n\nBut if you're going to use C, you have to actually write C.\n\n- --arw\n\n- -- \nA. Wilcox (awilfox)\nProject Lead, Adélie Linux\nhttp://adelielinux.org\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v2\n\niQIcBAEBCAAGBQJZvdapAAoJEMspy1GSK50UbkAP/1oSkfp9oUMTchdWbrOrA/R5\nV6zbXTzs2cUHFUwqTVG9I/L3bZoFkWJyzkh9YUIPbUa3YY9gTYZaH4bNEVzvF8cn\nPbYbcJ/wdTy8HGyXs6SuVENd/MZmk2FrDgv0+LyKQLqkkx8b4LA30sXdfuhVflg2\n2lRkKhrzlrEkV0Zp+YNaa8GJB4ewpRA9A3tkeTFTLq+Rj7cguaubKCFkkwguDF0d\ntEy43hwHUbZ7GT/UjBFKeODJGVQKoHcJ45JHdo2gRp9OtvKXJkcH07kjKPF7Z8Nc\n0ipGH9hvODT5tkkENexNiNCeWI9unbjqXjHsRg9Q5k5KsKlAhG33CAQxrEIVnBpT\nRMhq3S1To33b+Je7BqdPBwxJvDAhz8Pe7MSB6m8HcLW0DsD1t72s7utwkpgjSHlO\n4HRy68Nf7Q271OfulZtainVgLl7gU6HCbnm86dp+zk1WlJ/KOOtm5jz2NkruwrKV\n8QbPfIkV7LasB7XDOg/Q7TbObSmFYId869BkZU9N1oOzvgHpNVh7qgRdW5u65hlE\nYzZDwzdyWew3GjGnPLEi4hioqm4ZFdrahBujqWL+z4XUAblfm9i1gY5wDMb/ZYxm\noqDjguE3gnfrqpYVx5MBi6ow4Ykid/b1T2KfZQ4D92yLTdQ1SY9de435YsmkzZlU\nD9sGiVVcRyLqeqqBj5lV\n=ZJnT\n-----END PGP SIGNATURE-----\n"},{"id":"328242","messageId":"xmqq60ciui8o.fsf@gitster.mtv.corp.google.com","threadId":"46754","inReplyTo":"59BDD6AF.5090604@adelielinux.org","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-17T03:16:55Z","receivedAt":"2017-09-17T03:17:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"A. Wilcox\" writes:\n\n>> I did a quick scan for substring \"scanf\" and read through the \n>> output, and it seems that this is the only one that wants to do\n>> the this many characters, e.g. \"%42c\", conversion.\n>> So it seems to me that a real fix has to read the file ourselves\n>> and parse up to our HOST_NAME_MAX+1 to see if the hostname refers\n>> to us, and fscanf that cannot take \"slurp up to this many bytes\" is\n>> not useful tool to implementing that parsing.\n> ...\n> Except that is *exactly* *what* *s* *does* (quoting C11 §7.21.6.2):\n>\n> 9 An input item is defined as the longest sequence of input\n> characters which does not exceed any specified field width\n\nAh, sorry, I completely misread what you meant.\n\nI thought you were suggesting to replace \"%c\" with just an\nunadorned \"%s\". You meant that we can use \"%s\" instead.\nAnd that solution makes sense. Yes, it is exactly what %s\ndoes.\n\nSo something like the following would be a sufficient fix, I guess?\n\nThanks.\n\n-- >8 --\nSubject: gc: call fscanf() with %s, not %c, when reading hostname\n\nEarlier in this codepath, we (ab)used \"%c\" to read the hostname\nrecorded in the lockfile into locking_host[HOST_NAME_MAX + 1] while\nsubstituting with the actual value of HOST_NAME_MAX.\n\nThis turns out to be incorrect, as it an instruction to read exactly\nthe specified number of bytes. We are trying to read at most that\nmany bytes, we should be using \"%s\" instead.\n\nHelped-by: A. Wilcox \nSigned-off-by: Junio C Hamano \n---\n builtin/gc.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3c78fcb9b1..bb2d6c1fb2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -258,7 +258,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\tint should_exit;\n \n \t\tif (!scan_fmt)\n-\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX, HOST_NAME_MAX);\n+\t\t\tscan_fmt = xstrfmt(\"%s %%%ds\", \"%\"SCNuMAX, HOST_NAME_MAX);\n \t\tfp = fopen(pidfile_path, \"r\");\n \t\tmemset(locking_host, 0, sizeof(locking_host));\n \t\tshould_exit =\n"},{"id":"328243","messageId":"59BDEE1A.3000005@adelielinux.org","threadId":"46754","inReplyTo":"xmqq60ciui8o.fsf@gitster.mtv.corp.google.com","subject":"Re: Git 2.14.1: t6500: error during test on musl libc","fromName":"A. Wilcox","fromEmail":"awilfox@adelielinux.org","sentAt":"2017-09-17T03:38:02Z","receivedAt":"2017-09-17T03:38:11Z","isPatch":false,"sender":{"key":"awilfox@adelielinux.org","avatar":"https://gravatar.com/avatar/7dfeb1949d7201fcf186c1ef0d0d0664f667c03834d1630eb7d057e7d37c2266?d=mp&s=160"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA256\n\nOn 16/09/17 22:16, Junio C Hamano wrote:\n> Subject: gc: call fscanf() with %s, not %c, when reading\n> hostname\n> \n> Earlier in this codepath, we (ab)used \"%c\" to read the\n> hostname recorded in the lockfile into locking_host[HOST_NAME_MAX +\n> 1] while substituting with the actual value of\n> HOST_NAME_MAX.\n> \n> This turns out to be incorrect, as it an instruction to read\n> exactly\n\nit -> it is\n\n> the specified number of bytes. We are trying to read at most that \n> many bytes, we should be using \"%s\" instead.\n> \n> Helped-by: A. Wilcox Signed-off-by: Junio\n> C Hamano --- builtin/gc.c | 2 +- 1 file\n> changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c index\n> 3c78fcb9b1..bb2d6c1fb2 100644 --- a/builtin/gc.c +++\n> b/builtin/gc.c @@ -258,7 +258,7 @@ static const char\n> *lock_repo_for_gc(int force, pid_t* ret_pid) int should_exit;\n> \n> if (!scan_fmt) -\t\t\tscan_fmt = xstrfmt(\"%s %%%dc\", \"%\"SCNuMAX,\n> HOST_NAME_MAX); +\t\t\tscan_fmt = xstrfmt(\"%s %%%ds\", \"%\"SCNuMAX,\n> HOST_NAME_MAX); fp = fopen(pidfile_path, \"r\"); memset(locking_host,\n> 0, sizeof(locking_host)); should_exit =\n> \n\nAck. This is what I used in my testing; looks great. Thanks so much\nfor your time and patience.\n\n\nSincerely,\n- --arw\n\n- -- \nA. Wilcox (awilfox)\nProject Lead, Adélie Linux\nhttp://adelielinux.org\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v2\n\niQIcBAEBCAAGBQJZve4WAAoJEMspy1GSK50UruQP/3rca4kZp/mootkcgJrNwlSc\n5SvFETaBMYb9M6CewOIgDWtQVqdGmkX+vhlyz/fO1aMUzed9JNgoYD0Fj8S+8RL/\naan96+Om94znlWydSlU48ZaR69sbj012TSJBvQdAs9K9Nfi40lMVGi8BvI5vsAG0\nPCMyAUB4N6b9FYUNb6zO73JjmQSYzYV2TFOvACFgHwZ7ailyeyGI3LIP5Yd4OiF1\nERyJIKDoBjf0ns95xjox+HYFzG3VFDriM6GdEG1w25sLG+nvWxy/XV1Dv/K1/LiV\nVzSJ3FEdNdOoO5SLcX4uRMYzRKLt3ihwnwIS6SC44Xd7XaqaWpfpueGxilQCI3Yn\nFNWB3mX9oeXICIvM6PscJTzRLJd+gp3RbyLfavaQ2cNakrL9z+Qm5v6a52ufCqvP\nSGdruVLGLDCR0qPWokKa64+uSfi6QNNmVgzVx4fRIwbMSUm1+sEh0uIjqgTQPBTq\nJyn6og/T234punjBI+GuEXhsb3FEbJh2xGyOQbmhW4l/DPzerUpXJAgCPC2JT93+\nZ1s0aeDqC0n/dMHofVd8ZRFKt/ImVT0ywg7A9bJahhaJwmtkh0Xgb2hLgO5FGeuI\nzY+9FI+doH6Al+KgxqSduKfxDOsEoxYRLCRYO2QnNjE7iYLIdUYRXEucw3Z/VQA1\nb44AES8+WthuxQKsRstE\n=Zwpa\n-----END PGP SIGNATURE-----\n"}]}