[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <xmqqpoaqupo5.fsf@gitster.mtv.corp.google.com>
Date: Sun, 17 Sep 2017 09:36:26 +0900
From: Junio C Hamano <gitster@...ox.com>
To: "A. Wilcox" <awilfox@...lielinux.org>
Cc: Jeff King <peff@...f.net>, Kevin Daudt <me@...e.info>, git@...r.kernel.org, musl@...ts.openwall.com
Subject: Re: Git 2.14.1: t6500: error during test on musl libc
"A. Wilcox" <awilfox@...lielinux.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.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
