patchHandle double slashes in make_relative_path()

If you say

  git --git-dir=/some//path --work-tree=/some/ add <somefile>

then setup_work_tree() will call into make_relative_path() with abs="/some//path" and base="/some". (Note how the latter has already lost its trailing slash. One unfortunate user managed to trigger this because his $HOME ended in a slash.)

This means that when checking whether 'abs' is a path under 'base', we need to skip *two* slashes where the previous code only accounted for one. Fix it to handle an arbitrary number of slashes at that position.

Noticed-by: eldenz on freenode
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 path.c              |    6 +++---
 t/t1501-worktree.sh |    6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

Thomas Rast <trast@student.ethz.ch> writes:

Curious; why does your hunk header says set_shared_perm() while this is a patch to make_relative_path()? Do you run a broken git with funny funcname regexp pattern?

The function takes two paths, an early part of abs is supposed to match base; otherwise abs is not a path under base and the function returns the full path of abs.

Now what is the goal of this patch? To allow people to have duplicated slashes at random places in either abs or base, or is it only interested in a particular input that is malformed? If the latter, what is the permitted non-canonical input?

If abs were "/a//b/c" and base were "/a/b", then the combination is rejected by prefixcmp() and full "/a//b/c" is returned. Is it the intended behaviour of the patch?

I would actually have expected to see something like this, but I haven't even compile tested it, so...

 path.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

Ok, here is a compile and "make test" tested one, together with your addition to the test script.

I am still curious how you managed to end up with a wrong function name in the context header, though. The patch below has "set_shared_perm" because that is the header we find before the context of the hunk, so it is sort of understandable; we might want to squelch the hunk header string when the first context line of the hunk already matches the funcname pattern, though.

-- >8 --
Subject: ignore duplicated slashes in make_relative_path()

The function takes two paths, an early part of abs is supposed to match base; otherwise abs is not a path under base and the function returns the full path of abs. The caller can easily confuse the implementation by giving duplicated and needless slashes in these path arguments.

Credit for test script, motivation and initial patch goes to Thomas Rast, but the bugs in the implementation of this patch are mine..

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 path.c              |   32 +++++++++++++++++++++++---------
 t/t1501-worktree.sh |    6 ++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

Junio C Hamano schrieb:

And with this squashed in it has fewer of them ;-) and is more portable. The bug was that /foo was incorrectly stripped from /foobar.

-- Hannes

fredagen den 22 januari 2010 09.36.13 skrev  Johannes Sixt:

It seems this function does something unhealthy when you pass a path of the form //server/share. On windows dropping the double // at the beginning makes it a different path since // is the UNC prefix.

I'm not sure git on windows actually works with UNC-prefix anyway, so my point may be moot, but having even more places to fix to make it work doesn't help.

-- robin

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:

There is no problem in practice.

The function returns either the input unmodified, or it strips also at least one directory component, except when base is only "/" (or "//" or "///"...). I said in practice, because on Windows it does not make sense to invoke git with (literally)

   git --git-dir=//server/share/repo.git --work-tree=/ ...

i.e., without a drive prefix before the slash of --work-tree.

-- Hannes

lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:

Why not? //foo/bar/z is just as valid and useful a path as x:/z. 

Defining a drive-letter with msysgit is tricky because I have to find one that is available and then also restart every msys bash instance to make msys see it.

-- robin

On Samstag, 23. Januar 2010, Robin Rosenberg wrote:

Fortunately, make_relative_path() does not have the slightest problem with //foo/bar/z, either as value of abs (the path to make relative) or as base (the path to strip from abs).

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

If you did this:

    git --git-dir=//reposerver/repo.git --work-tree=//buildserver/workarea

then we would say "one is not a prefix of the other", so it would be fine. At least I don't think the "recover from unintentionally doubled slashes in user supplied path" fix is introducing any new problem in that case.

If on the other hand, if you did this:

    git --git-dir=//server/repo.git --work-tree=//server/workarea

that also would be Ok.

I think one issue is what happens when you did this:

    cd //server
    git --git-dir=//server/repo/repo.git --work-tree=repo

Does msysgit implementation figures out that the work tree is located at "//server/repo" when get_git_work_tree() is asked to produce an absolute path so that it can be compared with //server/repo/repo.git with the code? If it does (with the leading double slash), then "doubled slahses fix" is a regression we should do something about it. If it doesn't, then it probably doesn't matter.

Junio C Hamano <gitster@pobox.com> writes:

Nah, I wasn't thinking straight.  What happens if you did this?

	git --git-dir=//git/repo/repo.git --work-tree=/git/repo

where "//git/repo" is on the "git server" and you are working in local hierarchy "/git/repo"?

On Samstag, 23. Januar 2010, Junio C Hamano wrote:

Ah, right, this would not do the right thing. (But I can't verify this claim right now.)

The problem is that /git/repo without a drive prefix is a valid path and it means the path that begins at the same drive that CWD currently is. I would not dismiss this form of paths as too exotic, so we should care about them. OTOH, it can be worked around easily by the user (just insert the drive prefix). Dunno...

-- Hannes

Heya,

On Sat, Jan 23, 2010 at 21:41, Johannes Sixt <j6t@kdbg.org> wrote:

I think it's preferable to keep the old behavior where we fail if the user gives us an invalid argument, rather than fix a user error and break on a a valid argument instead. I think we should be correct first, and try and fix incorrect user behavior after.

On Saturday 23 January 2010 22:01:38 Sverre Rabbelier wrote:

I can't really comment on the Windows side of things, but I tried to come up with some more data points.

POSIX specifies that multiple slashes must be treated as if they were a single slash (except as in the next bullet point). Leading _double_ slashes may be treated implementation-dependently. [1] Non-leading double slashes do not seem to be specified.

There's a manpage path_resolution(7) on my system, which can also be found on the web quite easily, e.g. [2]. It doesn't say anything about multiple slashes, but experimentally my Linux resolves them as if they were single slashes (even a leading double slash).

Junio's patch is already in maint, so I suppose we're in the somewhat unfortunate situation where the old version didn't work in all cases on Linux, but the current one breaks on Windows in some cases. Then again, shouldn't windows get special support to figure out that /c/foo [3] is a prefix of /foo and vice versa, assuming you're currently in C:?

[1] http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11 Actually the server doesn't work for me, but google has a cached copy: http://209.85.129.132/search?q=cache:QUuajH7Dp5gJ:www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html+http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11&cd=1&hl=en&ct=clnk&gl=uk

[2] http://www.kernel.org/doc/man-pages/online/pages/man7/path_resolution.7.html

[3] Don't blame me if I didn't get that syntax right, I'm actively trying to forget I ever used Windows and it shows.

* Thomas Rast <trast@student.ethz.ch> [100124 14:58]:

http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266

| 3.266 Pathname
|
| A character string that is used to identify a file. In the context of
| IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes,
| including the terminating null byte. It has an optional beginning
| slash, followed by zero or more filenames separated by slashes. A
| pathname may optionally contain one or more trailing slashes.
| Multiple successive slashes are considered to be the same as one
| slash.

"Bernhard R. Link" <brlink@debian.org> writes:

Which is a bit stale, and there is an update that is crucial when discussing the issue that is the topic of this thread.

Try this one instead, especially the last ", except for" which is an important clarification.

http://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html

| 3.266 Pathname
| 
| A character string that is used to identify a file. In the context of
| POSIX.1-2008, a pathname may be limited to {PATH_MAX} bytes, including the
| terminating null byte. It has an optional beginning <slash>, followed by
| zero or more filenames separated by <slash> characters. A pathname may
| optionally contain one or more trailing <slash> characters. Multiple
| successive <slash> characters are considered to be the same as one
| <slash>, except for the case of exactly two leading <slash> characters.

And 4.12 in the same issue of POSIX says pathnames with exactly two leading slashes may be handled in an implemenation defined manner (i.e. three or more means the same thing as a single slash). Confusing ;-)

On Samstag, 23. Januar 2010, Johannes Sixt wrote:

I tested it, and it does the right thing. The reason is that before setup_work_tree() calls make_relative_path(), the --work-tree argument has been processed by make_absolute_path(), which adds the drive prefix.

As long as setup_work_tree() remains the only caller of make_relative_path(), we are safe.

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

Thanks; I think a more correct description of your findings is:

 - msysgit's make_absolute_path() does the right thing (i.e. adds "drive
   prefix" to "git/repo" given to --work-tree); and

 - as long as the callers feed what the platform considers absolute paths
   in abs and base, make_relative_path() does the right thing.

So I think we are Ok. We _might_ want to add Windows-only test at the beginning of make_relative_path() to make sure that the both inputs have double-slashes at the beginning to catch future broken callers, but I think that is a separate topic, and we don't have to do that as long as setup_work_tree(0 remains the only caller, as you said.

söndagen den 24 januari 2010 19.31.01 skrev  Junio C Hamano:

Separate patch posted since this problem occurs in more than one place. 

-- robin

On Friday 22 January 2010 02:40:41 Junio C Hamano wrote:

It's not about *permitted* input; the problem is simply that the current function gives back *bogus* paths, which causes git to fail. So I only went for the minimal patch to fix this.

Not handling the abs="/a//b/c" base="/a/b" case seemed ok to me since that was never turned as a relative "c", hence there would not be any speed loss (nor gain) from my patch.

Does that answer the question?

As for your patch, thanks for coming up with a real fix. I read the amended version, and it seems correct to me.

Thomas Rast <trast@student.ethz.ch> writes:

With that logic a minimal patch would have been not to call the function at all, as apparently the caller seem to be able to cope with absolute paths returned when they could be made relative, no?

In other words, it wasn't obvious to me if the minimal patch avoided returning a bogus result claiming that is a path relative to the base directory and instead returned an absolute path (which might be suboptimal but way better than giving a wrong thing back) in _all_ cases, or only just on _some_ cases but not others, and if it was the latter, what are the cases that it did better than the original.

By "amended", I take it to mean the fix-up by Hannes. I'll queue one for 'maint'.

Thanks.