[v2,3/3] shorten_unambiguous_ref(): avoid sscanf()

To shorten a fully qualified ref (e.g., taking "refs/heads/foo" to just
"foo"), we munge the usual lookup rules ("refs/heads/%.*s", etc) to drop
the ".*" modifier (so "refs/heads/%s"), and then use sscanf() to match
that against the refname, pulling the "%s" content into a separate
buffer.

This has a few downsides:

  - sscanf("%s") reportedly misbehaves on macOS with some input and
    locale combinations, returning a partial or garbled string. See
    this thread:

      https://lore.kernel.org/git/CAGF3oAcCi+fG12j-1U0hcrWwkF5K_9WhOi6ZPHBzUUzfkrZDxA@mail.gmail.com/

  - scanf's matching of "%s" is greedy. So the "refs/remotes/%s/HEAD"
    rule would never pull "origin" out of "refs/remotes/origin/HEAD".
    Instead it always produced "origin/HEAD", which is redundant with
    the "refs/remotes/%s" rule.

  - scanf in general is an error-prone interface. For example, scanning
    for "%s" will copy bytes into a destination string, which must have
    been correctly sized ahead of time to avoid a buffer overflow. In
    this case, the code is OK (the buffer is pessimistically sized to
    match the original string, which should give us a maximum). But in
    general, we do not want to encourage people to use scanf at all.

So instead, let's note that our lookup rules are not arbitrary format
strings, but all contain exactly one "%.*s" placeholder. We already rely
on this, both for lookup (we feed the lookup format along with exactly
one int/ptr combo to snprintf, etc) and for shortening (we munge "%.*s"
to "%s", and then insist that sscanf() finds exactly one result).

We can parse this manually by just matching the bytes that occur before
and after the "%.*s" placeholder. While we have a few extra lines of
parsing code, the result is arguably simpler, as can skip the
preprocessing step and its tricky memory management entirely.

The in-code comments should explain the parsing strategy, but there's
one subtle change here. The original code allocated a single buffer, and
then overwrote it in each loop iteration, since that's the only option
sscanf() gives us. But our parser can actually return a ptr/len combo
for the matched string, which is all we need (since we just feed it back
to the lookup rules with "%.*s"), and then copy it only when returning
to the caller.

There are a few new tests here, all using symbolic-ref (the code can be
triggered in many ways, but symrefs are convenient in that we don't need
to create a real ref, which avoids any complications from the filesystem
munging the name):

  - the first covers the real-world case which misbehaved on macOS.
    Setting LC_ALL is required to trigger the problem there (since
    otherwise our tests use LC_ALL=C), and hopefully is at worst simply
    ignored on other systems (and doesn't cause libc to complain, etc,
    on systems without that locale).

  - the second covers the "origin/HEAD" case as discussed above, which
    is now fixed

  - the remainder are for "weird" cases that work both before and after
    this patch, but would be easy to get wrong with off-by-one problems
    in the parsing (and came out of discussions and earlier iterations
    of the patch that did get them wrong).

  - absent here are tests of boring, expected-to-work cases like
    "refs/heads/foo", etc. Those are covered all over the test suite
    both explicitly (for-each-ref's refname:short) and implicitly (in
    the output of git-status, etc).

Reported-by: 孟子易 <mengziyi540841@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c                  | 78 +++++++++++++++++++++++------------------
 t/t1401-symbolic-ref.sh | 34 ++++++++++++++++++
 2 files changed, 77 insertions(+), 35 deletions(-)

Message ID	Y+z3RfhAxW/2iNYP@coredump.intra.peff.net (mailing list archive)
State	Accepted
Commit	613bef56b820cf7f24dc4b3ae65fc91826368185
Headers	show Return-Path: <git-owner@vger.kernel.org> Date: Wed, 15 Feb 2023 10:16:21 -0500 From: Jeff King <peff@peff.net> To: Eric Sunshine <sunshine@sunshineco.com> Cc: Junio C Hamano <gitster@pobox.com>, =?utf-8?b?5a2f5a2Q5piT?= <mengziyi540841@gmail.com>, git@vger.kernel.org Subject: [PATCH v2 3/3] shorten_unambiguous_ref(): avoid sscanf() Message-ID: <Y+z3RfhAxW/2iNYP@coredump.intra.peff.net> References: <Y+z3MtgayoXsxaHA@coredump.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <Y+z3MtgayoXsxaHA@coredump.intra.peff.net> Precedence: bulk
Series	get rid of sscanf() when shortening refs \| expand [v2,0/3] get rid of sscanf() when shortening refs [v2,1/3] shorten_unambiguous_ref(): avoid integer truncation [v2,2/3] shorten_unambiguous_ref(): use NUM_REV_PARSE_RULES constant [v2,3/3] shorten_unambiguous_ref(): avoid sscanf()

[v2,3/3] shorten_unambiguous_ref(): avoid sscanf()

Commit Message

Comments

Patch