Git 2.14.1: t6500: error during test on musl libc

Git 2.14.1: t6500: error during test on musl libc

[A. Wilcox]

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

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

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

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

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

[Jeff King]

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

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 5341 bytes --]

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

> c    Matches 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/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
-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
http://adelielinux.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gc-use-s-type-character-for-fscanf.patch --]
[-- Type: text/x-patch; name="0001-gc-use-s-type-character-for-fscanf.patch", Size: 1149 bytes --]

From afceb0f7755a87d0dd2194e95f26c9dc8f4bc688 Mon Sep 17 00:00:00 2001
From: "A. Wilcox" <AWilcox@Wilcox-Tech.com>
Date: Fri, 15 Sep 2017 23:55:57 -0500
Subject: [PATCH] gc: use 's' type character for fscanf

The ISO C standard states that using a field width together with the 'c'
type character will read the exact amount specified; if that amount of
bytes is not available, a match error occurs.

This patch allows the t6500 test to pass on the musl libc, and `git gc`
to behave correctly on syystems utilising musl.

Signed-off-by: A. Wilcox <AWilcox@Wilcox-Tech.com>
---
 builtin/gc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c78fcb..bb2d6c1 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 =
-- 
2.10.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [musl] Re: Git 2.14.1: t6500: error during test on musl libc

[Rich Felker]

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: Git 2.14.1: t6500: error during test on musl libc

[Junio C Hamano]

"A. Wilcox" <awilfox@adelielinux.org> 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]:
>
>> c    Matches 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: [musl] Re: Git 2.14.1: t6500: error during test on musl libc

[Szabolcs Nagy]

* Junio C Hamano <gitster@pobox.com> [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.

Re: Git 2.14.1: t6500: error during test on musl libc

[A. Wilcox]

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

On 16/09/17 19:36, Junio C Hamano wrote:
> "A. Wilcox" <awilfox@adelielinux.org> 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]:
>> 
>>> c    Matches 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 <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


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

Re: Git 2.14.1: t6500: error during test on musl libc

[Junio C Hamano]

"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.
>> 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 "%<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(-)

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

[A. Wilcox]

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

On 16/09/17 22:16, Junio C Hamano wrote:
> 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

it -> it is

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