Re: Git 2.14.1: t6500: error during test on musl libc
On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA256 > > Hi there, > > While bumping Git's version for our Linux distribution to 2.14.1, I've > run in to a new test failure in t6500-gc.sh. This is the output of > the failing test with debug=t verbose=t: This is a new test introduced by c45af94db (gc: run pre-detach operations under lock, 2017-07-11) which was included in v2.14.0. So it might be that this was already a problem for a longer time, only just recently uncovered. > > > expecting success: > # make sure we run a background auto-gc > test_commit make-pack && > git repack && > test_config gc.autopacklimit 1 && > test_config gc.autodetach true && > > # create a ref whose loose presence we can use to detect a > pack-refs run > git update-ref refs/heads/should-be-loose HEAD && > test_path_is_file .git/refs/heads/should-be-loose && > > # now fake a concurrent gc that holds the lock; we can use our > # shell pid so that it looks valid. > hostname=$(hostname || echo unknown) && > printf "$$ %s" "$hostname" >.git/gc.pid && > > # our gc should exit zero without doing anything > run_and_wait_for_auto_gc && > test_path_is_file .git/refs/heads/should-be-loose > > [master 28ecdda] make-pack > Author: A U Thor > 1 file changed, 1 insertion(+) > create mode 100644 make-pack.t > Counting objects: 3, done. > Delta compression using up to 8 threads. > Compressing objects: 100% (2/2), done. > Writing objects: 100% (3/3), done. > Total 3 (delta 0), reused 0 (delta 0) > Auto packing the repository in background for optimum performance. > See "git help gc" for manual housekeeping. > File .git/refs/heads/should-be-loose doesn't exist. > not ok 8 - background auto gc respects lock for all operations > # > # # make sure we run a background auto-gc > # test_commit make-pack && > # git repack && > # test_config gc.autopacklimit 1 && > # test_config gc.autodetach true && > # > # # create a ref whose loose presence we can use to > detect a pack-refs run > # git update-ref refs/heads/should-be-loose HEAD && > # test_path_is_file .git/refs/heads/should-be-loose && > # > # # now fake a concurrent gc that holds the lock; we can > use our > # # shell pid so that it looks valid. > # hostname=$(hostname || echo unknown) && > # printf "$$ %s" "$hostname" >.git/gc.pid && > # > # # our gc should exit zero without doing anything > # run_and_wait_for_auto_gc && > # test_path_is_file .git/refs/heads/should-be-loose > # > > # failed 1 among 8 test(s) > 1..8 > > > I admit I am mostly blind with the Git gc system. Should I use strace > on the git-gc process at the end? How would I accomplish that? Is > there a better way of debugging this error further? > > Core system stats: > > Intel x86_64 E3-1280 v3 @ 3.60 GHz > musl libc 1.1.16+20 > git 2.14.1, vanilla except for a patch to an earlier test due to > musl's inability to cope with EUC-JP > bash 4.3.48(1)-release > > Thank you very much. > > All the best, > - --arw > > - -- > A. Wilcox (awilfox) > Project Lead, Adélie Linux > http://adelielinux.org > -BEGIN PGP SIGNATURE- > Version: GnuPG v2 > > iQIcBAEBCAAGBQJZuz4yAAoJEMspy1GSK50UORwP/0Jxfp3xzexh27tSJlXYWS/g > g9QK8Xmid+3A0R696Vb2GguKg2roCcTmM2anR7iD1B2f2W31sgf+8M5mnJRHyJ1p > geEeqwrTdpCk6jQ/1Pj03L0NOftb1ftR6hcoVujBFAOph4jRlRdZDPA87fe6snrh > q99C3LoDXQcyK6WWJwzX+t2wOplKgpGJP8wTAaZ0AHoUwVS5CLPl8tP2XaY4kLfD > ZPPcvtp9wisVzzZ2ssE/CLGd38EbenNNZ6OJCBFJIHmlwey4G2isZ9kk6fVIHXi2 > unBJ8yVqI7hQKmQFSVQMMSFSd9azhHnDjTBO5mzWeRK9HNVMda3LZsXTtVeswnRs > lN/ASMdt5KdfpNy/plFB7yDWLlQSQY7j1mxBMR8lL3AdVVQUbJppDM795tt+rn6a > NCE2ESZMWd/QEULmT92AbkNJTj5ibBEoubnVTka05KMjaBLwIauhpqU5XxLFq2UH > y3JYQU9hm0E7dQE0CLXxIm5/574T6bBUgp1cXH3CjxkeUYKR1USVKtDfBV6t/Qmt > xlDZKPEfjKbTvL3KUF33G+eAp55wTwrJTaWlOp8A/JqooXavYghcsuFhYtCPJ8qo > fFUa8kBZP70E/O7JkycUu8wi7p42+j1a8gR6/AnPG2u2wyoiosLCxHX+nll4gKmN > b6BuiRn0Z9ie5xw4xcMR > =Vf8Z > -END PGP SIGNATURE-
Re: Git 2.14.1: t6500: error during test on musl libc
On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote: > On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote: > > -BEGIN PGP SIGNED MESSAGE- > > Hash: SHA256 > > > > Hi there, > > > > While bumping Git's version for our Linux distribution to 2.14.1, I've > > run in to a new test failure in t6500-gc.sh. This is the output of > > the failing test with debug=t verbose=t: > > This is a new test introduced by c45af94db > (gc: run pre-detach operations under lock, 2017-07-11) which was > included in v2.14.0. > > So it might be that this was already a problem for a longer time, only > just recently uncovered. > Adding Jeff King to CC
Re: Git 2.14.1: t6500: error during test on musl libc
On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote: > On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote: > > -BEGIN PGP SIGNED MESSAGE- > > Hash: SHA256 > > > > Hi there, > > > > While bumping Git's version for our Linux distribution to 2.14.1, I've > > run in to a new test failure in t6500-gc.sh. This is the output of > > the failing test with debug=t verbose=t: > > This is a new test introduced by c45af94db > (gc: run pre-detach operations under lock, 2017-07-11) which was > included in v2.14.0. > > So it might be that this was already a problem for a longer time, only > just recently uncovered. The code change there is not all that big. Mostly we're just checking that the lock is actually respected. The lock code doesn't exercise libc all that much. It does use fscanf, which I guess is a little exotic for us. It's also possible that hostname() doesn't behave quite as we expect. If you instrument gc like the patch below, what does it report when you run: GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8 I get: [...] trace: built-in: git 'gc' '--auto' Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. debug: gc lock already held by $my_hostname [...] If you get "acquired gc lock", then the problem is in lock_repo_for_gc(), and I'd suspect some problem with fscanf or hostname. -Peff --- diff --git a/builtin/gc.c b/builtin/gc.c index c22787ac72..a7450a0058 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -242,9 +242,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) int fd; char *pidfile_path; - if (is_tempfile_active(pidfile)) + if (is_tempfile_active(pidfile)) { /* already locked */ + trace_printf("debug: we already hold the gc lock"); return NULL; + } if (xgethostname(my_host, sizeof(my_host))) xsnprintf(my_host, sizeof(my_host), "unknown"); @@ -284,6 +286,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) rollback_lock_file(&lock); *ret_pid = pid; free(pidfile_path); + trace_printf("debug: gc lock already held by %s", locking_host); return locking_host; } } @@ -295,6 +298,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) commit_lock_file(&lock); pidfile = register_tempfile(pidfile_path); free(pidfile_path); + trace_printf("debug: we have acquired the gc lock"); return NULL; }
Re: Git 2.14.1: t6500: error during test on musl libc
On 15/09/17 06:30, Jeff King wrote: > On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote: > >> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote: >>> -BEGIN PGP SIGNED MESSAGE- >>> Hash: SHA256 >>> >>> Hi there, >>> >>> While bumping Git's version for our Linux distribution to 2.14.1, I've >>> run in to a new test failure in t6500-gc.sh. This is the output of >>> the failing test with debug=t verbose=t: >> >> This is a new test introduced by c45af94db >> (gc: run pre-detach operations under lock, 2017-07-11) which was >> included in v2.14.0. >> >> So it might be that this was already a problem for a longer time, only >> just recently uncovered. > > The code change there is not all that big. Mostly we're just checking > that the lock is actually respected. The lock code doesn't exercise libc > all that much. It does use fscanf, which I guess is a little exotic for > us. It's also possible that hostname() doesn't behave quite as we > expect. > > If you instrument gc like the patch below, what does it report when you > run: > > GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8 > > I get: > > [...] > trace: built-in: git 'gc' '--auto' > Auto packing the repository in background for optimum performance. > See "git help gc" for manual housekeeping. > debug: gc lock already held by $my_hostname > [...] > > If you get "acquired gc lock", then the problem is in > lock_repo_for_gc(), and I'd suspect some problem with fscanf or > hostname. > > -Peff Hey there Peff, What a corner-y corner case we have here. I believe the actual error is in the POSIX standard itself[1], as it is not clear what happens when there are not enough characters to 'fill' the width specified with %c in fscanf: > c >Matches a sequence of bytes of the number specified by the field > width (1 if no field width is present in the conversion > specification). I've tested a number of machines: * OpenBSD 5.7/amd64 * NetBSD 7.0/i386 * FreeBSD 12/PowerPC * glibc/arm * Windows 7 with Microsoft Visual C++ 2013 All of them will allow a so-called "short read" and give you as many characters as they can, treating the phrase "a sequence of bytes of the number specified" as meaning "*up to* the number". The musl libc treats this phrase as meaning "*exactly* the number", and fails if it cannot give you exactly the number you ask. IBM z/OS explicitly states in their documentation[2]: > Sequence of one or more characters as specified by field width And Microsoft similarly states[3]: > The width field is a positive decimal integer controlling the maximum > number of characters to be read for that field. No more than width > characters are converted and stored at the corresponding argument. > Fewer than width characters may be read if a whitespace character > (space, tab, or newline) or a character that cannot be converted > according to the given format occurs before width is reached. While musl's reading is correct from an English grammar point of view, it does not seem to be how any other implementation has read the standard. However! It gets better. The ISO C standard, committee draft version April 12, 2011, states[4]: > cMatches a sequence of characters of exactly the number specified > by the field width (1 if no field width is present in the directive). Since "[t]his volume of POSIX.1-2008 defers to the ISO C standard", it stands to reason that this is the intended meaning and behaviour. Thus meaning that literally every implementation, with the exception of the musl libc, is breaking the ISO C standard. Since Git is specifically attempting to read in a host name, there may be a solution: while 'c' guarantees that any byte will be read, and 's' will skip whitespace, RFCs 952 and 1123 §2.1[5] specify that a network host name must never contain whitespace. IDNA2008 §2.3.2.1[6] (and IDNA2003 before it) specifically removes ASCII whitespace characters from the valid set of Unicode codepoints for an IDNA host name[7]. Additionally, the buffer `locking_host` is already defined as an array of char of size HOST_NAME_MAX + 1, and the width specifier in fscanf is specified as HOST_NAME_MAX. Therefore, it should be safe to change git to use the 's' type character. Additionally, I can confirm that this change (patch attached) allows the Git test suite to pass on musl. I hope this message is informative. This was an exhausting, but necessary, exercise in trying to ensure code correctness. I am cc'ing the musl list so that this information may live there as well, in case someone in the future has issues with the 'c' type specifier with fscanf on musl. All the best, --arw [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/fscanf.html [2]: https://www.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.bpxbd00/fscanf.htm [3]: https://msdn.microsoft.com/en-us/library/xdb9w69d.aspx [4]: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf [5]: https://tools.ietf.org
Re: Git 2.14.1: t6500: error during test on musl libc
"A. Wilcox" writes: > While musl's reading is correct from an English grammar point of view, > it does not seem to be how any other implementation has read the standard. > > However! It gets better. > > The ISO C standard, committee draft version April 12, 2011, states[4]: > >> cMatches a sequence of characters of exactly the number specified >> by the field width (1 if no field width is present in the directive). > ... > Since Git is specifically attempting to read in a host name, there may > be a solution: while 'c' guarantees that any byte will be read, and 's' > will skip whitespace, RFCs 952 and 1123 §2.1[5] specify that a network > host name must never contain whitespace. IDNA2008 §2.3.2.1[6] (and > IDNA2003 before it) specifically removes ASCII whitespace characters > from the valid set of Unicode codepoints for an IDNA host name[7]. > Additionally, the buffer `locking_host` is already defined as an array > of char of size HOST_NAME_MAX + 1, and the width specifier in fscanf is > specified as HOST_NAME_MAX. Therefore, it should be safe to change git > to use the 's' type character. Additionally, I can confirm that this > change (patch attached) allows the Git test suite to pass on musl. I did a quick scan for substring "scanf" and read through the output, and it seems that this is the only one that wants to do the this many characters, e.g. "%42c", conversion. I am a bit worried about the correctness of your conclusion, though. As long as we are reading from the file written by us, because the string we write as the hostname part comes from what we prepare in my_host[HOST_NAME_MAX+1] using xgethostname(), we may know it would fit in locking_host[HOST_NAME_MAX+1]. But because HOST_NAME_MAX on my platform may be shorter than what your platform uses, I'll run over the end of my buffer if I am reading the lockfile you write to notice that the repository is in use from your host. After all, the reason why we write hostname in the file is because we expect the filesystem is shared across different hosts, so relying on HOST_NAME_MAX to be the same across platforms would not be a good way to go. So it seems to me that a real fix has to read the file ourselves and parse up to our HOST_NAME_MAX+1 to see if the hostname refers to us, and fscanf that cannot take "slurp up to this many bytes" is not useful tool to implementing that parsing. The current scan_fmt variable comes from da25bdb7 ("use HOST_NAME_MAX to size buffers for gethostname(2)", 2017-04-18), and before that, we used to use "%"SCNuMAX" %127c", which was already problematic. The "%127c" part came from the very original of this codepath in 64a99eb4 ("gc: reject if another gc is running, unless --force is given", 2013-08-08), whose first appearance in released versions was in v1.8.5, it seems. IOW, nobody tried to run Git with musl C in the past 4 years and you are the first one to notice? Thanks.
Re: Git 2.14.1: t6500: error during test on musl libc
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 16/09/17 19:36, Junio C Hamano wrote: > "A. Wilcox" writes: > >> While musl's reading is correct from an English grammar point of >> view, it does not seem to be how any other implementation has >> read the standard. >> >> However! It gets better. >> >> The ISO C standard, committee draft version April 12, 2011, >> states[4]: >> >>> cMatches a sequence of characters of exactly the number >>> specified by the field width (1 if no field width is present in >>> the directive). >> ... Since Git is specifically attempting to read in a host name, >> there may be a solution: while 'c' guarantees that any byte will >> be read, and 's' will skip whitespace, RFCs 952 and 1123 §2.1[5] >> specify that a network host name must never contain whitespace. >> IDNA2008 §2.3.2.1[6] (and IDNA2003 before it) specifically >> removes ASCII whitespace characters from the valid set of Unicode >> codepoints for an IDNA host name[7]. Additionally, the buffer >> `locking_host` is already defined as an array of char of size >> HOST_NAME_MAX + 1, and the width specifier in fscanf is specified >> as HOST_NAME_MAX. Therefore, it should be safe to change git to >> use the 's' type character. Additionally, I can confirm that >> this change (patch attached) allows the Git test suite to pass on >> musl. > > I did a quick scan for substring "scanf" and read through the > output, and it seems that this is the only one that wants to do > the this many characters, e.g. "%42c", conversion. > > I am a bit worried about the correctness of your conclusion, > though. %[width]s means read up to [width] non-ws characters. It is exactly what Git wants out of %[width]c, with the difference that 's' will stop at whitespace. Since hostnames cannot legally contain whitespace, the difference is negligible. > As long as we are reading from the file written by us, because the > string we write as the hostname part comes from what we prepare in > my_host[HOST_NAME_MAX+1] using xgethostname(), we may know it > would fit in locking_host[HOST_NAME_MAX+1]. But because > HOST_NAME_MAX on my platform may be shorter than what your platform > uses, I'll run over the end of my buffer if I am reading the > lockfile you write to notice that the repository is in use from > your host. After all, the reason why we write hostname in the file > is because we expect the filesystem is shared across different > hosts, so relying on HOST_NAME_MAX to be the same across platforms > would not be a good way to go. You cannot run over the buffer in any scenario: if the hostname is longer than [width], then [width] + \0 will be written to the buffer. Since the buffer is HOST_NAME_MAX+1 and the width is HOST_NAME_MAX, it stands to reason that you cannot overflow the buffer. > So it seems to me that a real fix has to read the file ourselves > and parse up to our HOST_NAME_MAX+1 to see if the hostname refers > to us, and fscanf that cannot take "slurp up to this many bytes" is > not useful tool to implementing that parsing. Except that is *exactly* *what* *s* *does* (quoting C11 §7.21.6.2): 9 An input item is defined as the longest sequence of input characters which does not exceed any specified field width s Matches a sequence of non-white-space characters. awilcox on elaine ~ $ cat -> myhost elaine.foxkit.us awilcox on elaine ~ $ cat -> test.c #include int main(void) { char test[9]; fscanf(fopen("myhost", "r"), "%8s", test); printf("result: %s\n", test); return 0; } awilcox on elaine ~ $ c99 -o test test.c awilcox on elaine ~ $ ./test result: elaine.f awilcox on elaine ~ $ /usr/lib/libc.so musl libc (powerpc) Version 1.1.16 > The current scan_fmt variable comes from da25bdb7 ("use > HOST_NAME_MAX to size buffers for gethostname(2)", 2017-04-18), > and before that, we used to use "%"SCNuMAX" %127c", which was > already problematic. The "%127c" part came from the very original > of this codepath in 64a99eb4 ("gc: reject if another gc is running, > unless --force is given", 2013-08-08), whose first appearance in > released versions was in v1.8.5, it seems. IOW, nobody tried to > run Git with musl C in the past 4 years and you are the first one > to notice? Yes. Yes I am. Because this is the first version that has a *test* that *tests* this behaviour. The odds that someone: * is using musl * with a Git repository over a network file system * with another platform with a different HOST_NAME_MAX * both concurrently running `git gc`, or the non-musl one being killed * with loose refs that they, for some reason, would notice not being gc'd properly is almost zero. However, it is theoretically something that could happen, and that is the point of a test suite: testing code paths that are rarely used to ensure they are correct when needed. And this test has proven that this code is *not* correct and will *not* function as intended when it is needed. The fact that this c
Re: Git 2.14.1: t6500: error during test on musl libc
"A. Wilcox" writes: >> I did a quick scan for substring "scanf" and read through the >> output, and it seems that this is the only one that wants to do >> the this many characters, e.g. "%42c", conversion. >> So it seems to me that a real fix has to read the file ourselves >> and parse up to our HOST_NAME_MAX+1 to see if the hostname refers >> to us, and fscanf that cannot take "slurp up to this many bytes" is >> not useful tool to implementing that parsing. > ... > Except that is *exactly* *what* *s* *does* (quoting C11 §7.21.6.2): > > 9 An input item is defined as the longest sequence of input > characters which does not exceed any specified field width Ah, sorry, I completely misread what you meant. I thought you were suggesting to replace "%c" with just an unadorned "%s". You meant that we can use "%s" instead. And that solution makes sense. Yes, it is exactly what %s does. So something like the following would be a sufficient fix, I guess? Thanks. -- >8 -- Subject: gc: call fscanf() with %s, not %c, when reading hostname Earlier in this codepath, we (ab)used "%c" to read the hostname recorded in the lockfile into locking_host[HOST_NAME_MAX + 1] while substituting with the actual value of HOST_NAME_MAX. This turns out to be incorrect, as it an instruction to read exactly the specified number of bytes. We are trying to read at most that many bytes, we should be using "%s" instead. Helped-by: A. Wilcox Signed-off-by: Junio C Hamano --- builtin/gc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/gc.c b/builtin/gc.c index 3c78fcb9b1..bb2d6c1fb2 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -258,7 +258,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) int should_exit; if (!scan_fmt) - scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX, HOST_NAME_MAX); + scan_fmt = xstrfmt("%s %%%ds", "%"SCNuMAX, HOST_NAME_MAX); fp = fopen(pidfile_path, "r"); memset(locking_host, 0, sizeof(locking_host)); should_exit =
Re: Git 2.14.1: t6500: error during test on musl libc
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 16/09/17 22:16, Junio C Hamano wrote: > Subject: gc: call fscanf() with %s, not %c, when reading > hostname > > Earlier in this codepath, we (ab)used "%c" to read the > hostname recorded in the lockfile into locking_host[HOST_NAME_MAX + > 1] while substituting with the actual value of > HOST_NAME_MAX. > > This turns out to be incorrect, as it an instruction to read > exactly it -> it is > the specified number of bytes. We are trying to read at most that > many bytes, we should be using "%s" instead. > > Helped-by: A. Wilcox Signed-off-by: Junio > C Hamano --- builtin/gc.c | 2 +- 1 file > changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/gc.c b/builtin/gc.c index > 3c78fcb9b1..bb2d6c1fb2 100644 --- a/builtin/gc.c +++ > b/builtin/gc.c @@ -258,7 +258,7 @@ static const char > *lock_repo_for_gc(int force, pid_t* ret_pid) int should_exit; > > if (!scan_fmt) - scan_fmt = xstrfmt("%s %%%dc", > "%"SCNuMAX, > HOST_NAME_MAX); + scan_fmt = xstrfmt("%s %%%ds", > "%"SCNuMAX, > HOST_NAME_MAX); fp = fopen(pidfile_path, "r"); memset(locking_host, > 0, sizeof(locking_host)); should_exit = > Ack. This is what I used in my testing; looks great. Thanks so much for your time and patience. Sincerely, - --arw - -- A. Wilcox (awilfox) Project Lead, Adélie Linux http://adelielinux.org -BEGIN PGP SIGNATURE- Version: GnuPG v2 iQIcBAEBCAAGBQJZve4WAAoJEMspy1GSK50UruQP/3rca4kZp/mootkcgJrNwlSc 5SvFETaBMYb9M6CewOIgDWtQVqdGmkX+vhlyz/fO1aMUzed9JNgoYD0Fj8S+8RL/ aan96+Om94znlWydSlU48ZaR69sbj012TSJBvQdAs9K9Nfi40lMVGi8BvI5vsAG0 PCMyAUB4N6b9FYUNb6zO73JjmQSYzYV2TFOvACFgHwZ7ailyeyGI3LIP5Yd4OiF1 ERyJIKDoBjf0ns95xjox+HYFzG3VFDriM6GdEG1w25sLG+nvWxy/XV1Dv/K1/LiV VzSJ3FEdNdOoO5SLcX4uRMYzRKLt3ihwnwIS6SC44Xd7XaqaWpfpueGxilQCI3Yn FNWB3mX9oeXICIvM6PscJTzRLJd+gp3RbyLfavaQ2cNakrL9z+Qm5v6a52ufCqvP SGdruVLGLDCR0qPWokKa64+uSfi6QNNmVgzVx4fRIwbMSUm1+sEh0uIjqgTQPBTq Jyn6og/T234punjBI+GuEXhsb3FEbJh2xGyOQbmhW4l/DPzerUpXJAgCPC2JT93+ Z1s0aeDqC0n/dMHofVd8ZRFKt/ImVT0ywg7A9bJahhaJwmtkh0Xgb2hLgO5FGeuI zY+9FI+doH6Al+KgxqSduKfxDOsEoxYRLCRYO2QnNjE7iYLIdUYRXEucw3Z/VQA1 b44AES8+WthuxQKsRstE =Zwpa -END PGP SIGNATURE-
Re: [musl] Re: Git 2.14.1: t6500: error during test on musl libc
On Fri, Sep 15, 2017 at 11:58:41PM -0500, A. Wilcox wrote: > On 15/09/17 06:30, Jeff King wrote: > > On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote: > > > >> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote: > >>> -BEGIN PGP SIGNED MESSAGE- > >>> Hash: SHA256 > >>> > >>> Hi there, > >>> > >>> While bumping Git's version for our Linux distribution to 2.14.1, I've > >>> run in to a new test failure in t6500-gc.sh. This is the output of > >>> the failing test with debug=t verbose=t: > >> > >> This is a new test introduced by c45af94db > >> (gc: run pre-detach operations under lock, 2017-07-11) which was > >> included in v2.14.0. > >> > >> So it might be that this was already a problem for a longer time, only > >> just recently uncovered. > > > > The code change there is not all that big. Mostly we're just checking > > that the lock is actually respected. The lock code doesn't exercise libc > > all that much. It does use fscanf, which I guess is a little exotic for > > us. It's also possible that hostname() doesn't behave quite as we > > expect. > > > > If you instrument gc like the patch below, what does it report when you > > run: > > > > GIT_TRACE=1 ./t6500-gc.sh --verbose-only=8 > > > > I get: > > > > [...] > > trace: built-in: git 'gc' '--auto' > > Auto packing the repository in background for optimum performance. > > See "git help gc" for manual housekeeping. > > debug: gc lock already held by $my_hostname > > [...] > > > > If you get "acquired gc lock", then the problem is in > > lock_repo_for_gc(), and I'd suspect some problem with fscanf or > > hostname. > > > > -Peff > > > Hey there Peff, > > What a corner-y corner case we have here. I believe the actual error is > in the POSIX standard itself[1], as it is not clear what happens when > there are not enough characters to 'fill' the width specified with %c in > fscanf: ISO C specifies very clearly what happens, in 7.21.6.2 The fscanf function, paragraph 12: c Matches a sequence of characters of exactly the number specified by the field width... Note the word "exactly". Thus a read of fewer characters is not a match. There is an open glibc bug for this with classic Drepper behavior until his departure, followed by acknowledgement of the bug, but no further action I'm aware of: https://sourceware.org/bugzilla/show_bug.cgi?id=12701 Any applications depending on the buggy glibc behavior should be fixed. Rich
Re: [musl] Re: Git 2.14.1: t6500: error during test on musl libc
* Junio C Hamano [2017-09-17 09:36:26 +0900]: > versions was in v1.8.5, it seems. IOW, nobody tried to run Git with > musl C in the past 4 years and you are the first one to notice? git works fine on musl in practice, i use it for more than 4years now.