git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Adam Dinwoodie <adam@dinwoodie.org>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: RyotaK <security@ryotak.me>, Git Mailing List <git@vger.kernel.org>
Subject: Re: [RFC PATCH] cygwin: disallow backslashes in file names
Date: Tue, 27 Apr 2021 20:22:49 +0100	[thread overview]
Message-ID: <CA+kUOamYmFcKA+_on83=EbitvL4FQo9teMEbRHsQ=xo2ave1yQ@mail.gmail.com> (raw)
In-Reply-To: <CA+kUOan3vk1zJezpieRhKwZ8gsYrCxDBefkXJ1fUC61O+gb12A@mail.gmail.com>

[Re-adding the previous Cc list that I'd failed to copy on my previous
email, sorry!]

On Mon, 26 Apr 2021 at 20:56, Adam Dinwoodie wrote:
>
> On Mon, 26 Apr 2021 at 15:08, Johannes Schindelin wrote:
> >
> > Hi Adam,
> >
> > On Sat, 24 Apr 2021, Adam Dinwoodie wrote:
> > > Notes:
> > >     The patch to read-cache.c is the one I've applied downstream as the Cygwin Git
> > >     maintainer to resolve this vulnerability, and I've manually tested that it
> > >     resolves the vulnerability, so that's the change I'd recommend anyone who needs
> > >     to build Git on Cygwin themselves take until there's something officially in
> > >     the Git source code.
> > >
> > >     I'm much less convinced by my approach for the test script.  I definitely think
> > >     it's worth having a test here, but the test as written still fails, as the test
> > >     seems to be looking for the error message "directory not empty", but running
> > >     the test on Cygwin produces the error "cannot create submodule directory d\a".
> > >     I'm not sure why that difference exists, and whether the correct approach would
> > >     be to (a) ensure the error messages are consistent across platforms or (b) to
> > >     change the test to expect the appropriate error on the appropriate platform.
> >
> > Wasn't there something in Cygwin that _allowed_ backslashes as file name
> > characters? I vaguely remember that the ASCII characters forbidden by
> > Windows were mapped into some "private page".
> >
> > Maybe that is responsible for the difference here?
>
> So there is special handling of a bunch of characters like ":" that
> are valid as parts of filenames on most *nix systems, but which aren't
> valid on Windows, by substituting them for characters in the Unicode
> "private use area" space. Backslash isn't one of those characters,
> though; quoting
> https://cygwin.com/cygwin-ug-net/using-specialnames.html (which I just
> checked myself to be sure): "The backslash has to be exempt from this
> conversion, because Cygwin accepts Win32 filenames including
> backslashes as path separators on input."
>
> Which is not to say this special handling _isn't_ the cause of the
> difference here, but it's not so simple as that. If nobody spots an
> explanation I've missed, I'll start digging into the code and strace
> to work out exactly what's causing the difference in behaviour.

I've worked out what's going wrong here: the "prevent git~1 squatting
on Windows" test is actually testing a selection of different Windows
path oddities, which are handled differently between Git for Windows
and Cygwin Git. The specific behaviour here is the handling of a
directory called "d."; Git for Windows (I assume in the MSYS2 layer)
follows the standard Windows convention of treating "d." and "d" as
identical filenames, while Cygwin sticks to its general design
philosophy of mostly emulating *nix systems, allowing objects with
both filenames to exist in the same directory (and causing pain for
most non-Cygwin applications that try to interact with them).

Essentially this test is checking a bunch of different oddities about
path handling on Windows. Some things – such as handling backslashes –
are common to both Cygwin and MSYS2; some – such as handling trailing
periods – aren't. So I expect the solution here will be to have
separate tests for (a) Git for Windows, (b) Cygwin Git, and (c) common

behaviour.

> > >     I'm also not convinced by my approach of adding a "WINDOWS" prerequisite to
> > >     test-lib.sh. I went with this as I couldn't immediately see a way to pass
> > >     prerequisites on an "any" rather than "all" basis to test_expect_success, and
> > >     this would allow us to simplify all the tests that currently have
> > >     "!MINGW,!CYGWIN" as prerequisites, but it still feels a bit clunky to me.
> >
> > Right, the only way I could think of it would be
> >
> >         test_lazy_prereq 'test_have_prereq MINGW || test_have_prereq CYGWIN'
> >
> > Your approach looks fine to me, though.
>
> Grand, okay. I'll stick with that for now, then, and follow up with a
> patch to tidy up the other prerequisites at some point in the future.
>
> Adam

  parent reply	other threads:[~2021-04-27 19:23 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-24 21:21 Adam Dinwoodie
2021-04-25  2:22 ` Johannes Schindelin
     [not found]   ` <CA+kUOan3vk1zJezpieRhKwZ8gsYrCxDBefkXJ1fUC61O+gb12A@mail.gmail.com>
2021-04-27 19:22     ` Adam Dinwoodie [this message]
2021-04-28  0:27       ` Johannes Schindelin
2021-04-29 20:11 ` [PATCH] " Adam Dinwoodie
2021-04-30  0:48   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CA+kUOamYmFcKA+_on83=EbitvL4FQo9teMEbRHsQ=xo2ave1yQ@mail.gmail.com' \
    --to=adam@dinwoodie.org \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=security@ryotak.me \
    --subject='Re: [RFC PATCH] cygwin: disallow backslashes in file names' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox