Git 2.14.1: t6500: error during test on musl libc

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

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 <author@example.com>
 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-----

On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:

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.

On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:

Adding Jeff King to CC

On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:

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

---

On 15/09/17 06:30, Jeff King wrote:

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:

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

And Microsoft similarly states[3]:

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

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/html/rfc1123#section-2.1 [6]: https://tools.ietf.org/html/rfc5890#section-2.3.2.1 [7]: http://unicode.org/faq/idn.html#33

On Fri, Sep 15, 2017 at 11:58:41PM -0500, A. Wilcox wrote:

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

"A. Wilcox" <awilfox@adelielinux.org> 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.

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.

* Junio C Hamano <gitster@pobox.com> [2017-09-17 09:36:26 +0900]:

git works fine on musl in practice, i use it for more than 4years now.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 16/09/17 19:36, Junio C Hamano wrote:

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

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.

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

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 code "hasn't caused issues until now" does not mean that it is correct; it only means that you are lucky enough that nobody has attempted to use it until now.

Like it or not, the ISO C standard states exactly how fscanf should behave, and you are relying on non-standard behaviour for a *lock* file.

If you are uninterested in conforming to the standard of the language in which the source control system is written in (C), then perhaps it should be written in a different language. Perhaps Perl, or Rust.

But if you're going to use C, you have to actually write C.

- --arw

- -- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
http://adelielinux.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJZvdapAAoJEMspy1GSK50UbkAP/1oSkfp9oUMTchdWbrOrA/R5 V6zbXTzs2cUHFUwqTVG9I/L3bZoFkWJyzkh9YUIPbUa3YY9gTYZaH4bNEVzvF8cn PbYbcJ/wdTy8HGyXs6SuVENd/MZmk2FrDgv0+LyKQLqkkx8b4LA30sXdfuhVflg2 2lRkKhrzlrEkV0Zp+YNaa8GJB4ewpRA9A3tkeTFTLq+Rj7cguaubKCFkkwguDF0d tEy43hwHUbZ7GT/UjBFKeODJGVQKoHcJ45JHdo2gRp9OtvKXJkcH07kjKPF7Z8Nc 0ipGH9hvODT5tkkENexNiNCeWI9unbjqXjHsRg9Q5k5KsKlAhG33CAQxrEIVnBpT RMhq3S1To33b+Je7BqdPBwxJvDAhz8Pe7MSB6m8HcLW0DsD1t72s7utwkpgjSHlO 4HRy68Nf7Q271OfulZtainVgLl7gU6HCbnm86dp+zk1WlJ/KOOtm5jz2NkruwrKV 8QbPfIkV7LasB7XDOg/Q7TbObSmFYId869BkZU9N1oOzvgHpNVh7qgRdW5u65hlE YzZDwzdyWew3GjGnPLEi4hioqm4ZFdrahBujqWL+z4XUAblfm9i1gY5wDMb/ZYxm oqDjguE3gnfrqpYVx5MBi6ow4Ykid/b1T2KfZQ4D92yLTdQ1SY9de435YsmkzZlU D9sGiVVcRyLqeqqBj5lV =ZJnT -----END PGP SIGNATURE-----

"A. Wilcox" <awilfox@adelielinux.org> writes:

Ah, sorry, I completely misread what you meant.

I thought you were suggesting to replace "%<length>c" with just an unadorned "%s". You meant that we can use "%<length>s" instead. And that solution makes sense. Yes, it is exactly what %<len>s does.

So something like the following would be a sufficient fix, I guess?

Thanks.

-- >8 --
Subject: gc: call fscanf() with %<len>s, not %<len>c, when reading hostname

Earlier in this codepath, we (ab)used "%<len>c" to read the hostname recorded in the lockfile into locking_host[HOST_NAME_MAX + 1] while substituting <len> 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 "%<len>s" instead.

Helped-by: A. Wilcox <awilfox@adelielinux.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/gc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 16/09/17 22:16, Junio C Hamano wrote:

it -> it is

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