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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 17, 2017, 00:36 UTC
Message-ID: <xmqqpoaqupo5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <59BCAF81.3090206@adelielinux.org>

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

Show 21 quoted lines

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