[PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

The sed provided by Solaris in /usr/xpg4/bin has a bug whereby an
unanchored regex using * for zero or more repetitions sees two
separate matches fed to the substitution engine in some cases.

This is evidenced by:

$ for sed in /usr/xpg4/bin/sed /usr/bin/sed /opt/csw/gnu/sed; do \
echo 'ab' | $sed -e 's|[a]*|X|g'; \
done
XXbX
XbX
XbX

This bug was triggered during a git submodule clone operation as
exercised in the setup stage of t5526-fetch-submodules when using the
default SANE_TOOL_PATH for Solaris.  It led to paths such as
..../.. being used in the submodule .git gitdir reference.

Using the expression 's|\([^/]*\(/*\)\)|..\2|g' provides the desired
result with all three three tested sed implementations but is harder
to read.  Instead, use an additional -e script to clean up after the
bug on Solaris.  The second script will be a functional no-op on most
sed implementations.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 git-submodule.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index efc86ad..0adad22 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,10 +167,12 @@ module_clone()
 	a=${a%/}
 	b=${b%/}
 
-	rel=$(echo $b | sed -e 's|[^/]*|..|g')
+	# Note: The second -e is to work around a bug in Solairs'
+	# xpg4/sed.  It will be a no-op in a working implementation.
+	rel=$(echo $b | sed -e 's|[^/]*|..|g' -e 's|\.\{4\}|..|g')
 	echo "gitdir: $rel/$a" >"$path/.git"
 
-	rel=$(echo $a | sed -e 's|[^/]*|..|g')
+	rel=$(echo $a | sed -e 's|[^/]*|..|g' -e 's|\.\{4\}|..|g')
 	(clear_local_git_env; cd "$path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
-- 
1.7.9

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Andreas Schwab]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> The sed provided by Solaris in /usr/xpg4/bin has a bug whereby an
> unanchored regex using * for zero or more repetitions sees two
> separate matches fed to the substitution engine in some cases.
>
> This is evidenced by:
>
> $ for sed in /usr/xpg4/bin/sed /usr/bin/sed /opt/csw/gnu/sed; do \
> echo 'ab' | $sed -e 's|[a]*|X|g'; \
> done
> XXbX
> XbX
> XbX
>
> This bug was triggered during a git submodule clone operation as
> exercised in the setup stage of t5526-fetch-submodules when using the
> default SANE_TOOL_PATH for Solaris.  It led to paths such as
> ..../.. being used in the submodule .git gitdir reference.
>
> Using the expression 's|\([^/]*\(/*\)\)|..\2|g' provides the desired

How about using 's|[^/][^/]*|..|g' instead, which should avoid the bug
as well.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

Excerpts from Andreas Schwab's message of Mon Apr 09 02:40:03 -0400 2012:

> How about using 's|[^/][^/]*|..|g' instead, which should avoid the bug
> as well.

I'd be ok with that change if the changed semantics of the regex are
ok in this application.  It's essentially the same as s|[^/]+|..|g,
which requires at least one character.

In the current code, if you do:

echo '/' | sed -e 's|[^]*|..|g'

you get: ../.. (from a working implementation).

Your regex would see the result  be: /

I don't think we'd ever be passing a plain /, but we might pass a
fully qualified path /path/to/foo, which would see the result change
from ../../../.. to /../../.. and that could have unintended impact.

I'd need to look more closely to see if this is a problem in reality.
Others more familiar with this code likely know the answer already.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Andreas Schwab]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> Excerpts from Andreas Schwab's message of Mon Apr 09 02:40:03 -0400 2012:
>
>> How about using 's|[^/][^/]*|..|g' instead, which should avoid the bug
>> as well.
>
> I'd be ok with that change if the changed semantics of the regex are
> ok in this application.  It's essentially the same as s|[^/]+|..|g,
> which requires at least one character.
>
> In the current code, if you do:
>
> echo '/' | sed -e 's|[^]*|..|g'
>
> you get: ../.. (from a working implementation).
>
> Your regex would see the result  be: /
>
> I don't think we'd ever be passing a plain /, but we might pass a
> fully qualified path /path/to/foo, which would see the result change
> from ../../../.. to /../../.. and that could have unintended impact.

AFAICS the variables at this point never contain a value with a leading
slash.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Ben Walton <bwalton@artsci.utoronto.ca> writes:
>
>> Excerpts from Andreas Schwab's message of Mon Apr 09 02:40:03 -0400 2012:
>>
>>> How about using 's|[^/][^/]*|..|g' instead, which should avoid the bug
>>> as well.
>>
>> I'd be ok with that change if the changed semantics of the regex are
>> ok in this application.  It's essentially the same as s|[^/]+|..|g,
>> which requires at least one character.
>>
>> In the current code, if you do:
>>
>> echo '/' | sed -e 's|[^]*|..|g'
>>
>> you get: ../.. (from a working implementation).
>>
>> Your regex would see the result  be: /
>>
>> I don't think we'd ever be passing a plain /, but we might pass a
>> fully qualified path /path/to/foo, which would see the result change
>> from ../../../.. to /../../.. and that could have unintended impact.
>
> AFAICS the variables at this point never contain a value with a leading
> slash.

Correct.  After $a and $b are initialized to "$(cd somewhere ; pwd)/",
there is a loop:

	while test "${a%%/*}" = "${b%%/*}"
	do
		a=${a#*/}
		b=${b#*/}
	done

The test for the first iteration will see that they both start with '/' so
stripping maximal trailing substring that match "/*" from them will yield
two empty strings, and we always enter the body of the loop.  The body
during the first iteration will strip the leading '/'.

When pwd computed for both $a and $b were at '/', this loop may do
something funky ($b will become empty), but that is covered by an earlier
"is $a inside $b or vice versa?" check, so we should be OK.

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

Excerpts from Andreas Schwab's message of Mon Apr 09 11:09:13 -0400 2012:

> >> How about using 's|[^/][^/]*|..|g' instead, which should avoid the bug
> >> as well.
>
> AFAICS the variables at this point never contain a value with a leading
> slash.

Ok.  Junio also agrees with this and the test suite passes too. I'll
send in an updated patch shortly.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

[PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

The sed provided by Solaris in /usr/xpg4/bin has a bug whereby an
unanchored regex using * for zero or more repetitions sees two
separate matches fed to the substitution engine in some cases.

This is evidenced by:

$ for sed in /usr/xpg4/bin/sed /usr/bin/sed /opt/csw/gnu/sed; do \
echo 'ab' | $sed -e 's|[a]*|X|g'; \
done
XXbX
XbX
XbX

This bug was triggered during a git submodule clone operation as
exercised in the setup stage of t5526-fetch-submodules when using the
default SANE_TOOL_PATH for Solaris.  It led to paths such as
..../.. being used in the submodule .git gitdir reference.

Using the expression 's|\([^/]*\(/*\)\)|..\2|g' provides the desired
result with all three three tested sed implementations but is harder
to read.  As we do not need to handle fully qualified paths though,
the expression could actually be [^/]+ which isn't properly handled
either.  Instead, use [^/][^/]*, as suggested by Andreas Schwab, which
works on all three tested sed implementations.

The new expression is semantically different than the original one.
It will not place a leading '..' on a fully qualified path as the
original expression did.  All of the paths being passed through this
regex are relative and did not rely on this behaviour so it's a safe
change.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 git-submodule.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index efc86ad..de972e7 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,10 +167,12 @@ module_clone()
 	a=${a%/}
 	b=${b%/}
 
-	rel=$(echo $b | sed -e 's|[^/]*|..|g')
+	# Note: The extra [^/] is to work around a bug in Solairs'
+	# xpg4/sed.
+	rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
 	echo "gitdir: $rel/$a" >"$path/.git"
 
-	rel=$(echo $a | sed -e 's|[^/]*|..|g')
+	rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
 	(clear_local_git_env; cd "$path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
-- 
1.7.4.1

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Andreas Schwab]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> +	# Note: The extra [^/] is to work around a bug in Solairs'

s/Solairs/Solaris/

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Ben Walton <bwalton@artsci.utoronto.ca> writes:
>
>> +	# Note: The extra [^/] is to work around a bug in Solairs'
>
> s/Solairs/Solaris/

True, but we do not have to single out the bug.  The resulting code is
easier to understand as it always expect at least one letter between
slashes, which holds true because we are handling output from pwd.

I'd rewrite it to:

	# Turn each leading "*/" component into "../"

or something like that.

[PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

The sed provided by Solaris in /usr/xpg4/bin has a bug whereby an
unanchored regex using * for zero or more repetitions sees two
separate matches fed to the substitution engine in some cases.

This is evidenced by:

$ for sed in /usr/xpg4/bin/sed /usr/bin/sed /opt/csw/gnu/sed; do \
echo 'ab' | $sed -e 's|[a]*|X|g'; \
done
XXbX
XbX
XbX

This bug was triggered during a git submodule clone operation as
exercised in the setup stage of t5526-fetch-submodules when using the
default SANE_TOOL_PATH for Solaris.  It led to paths such as
..../.. being used in the submodule .git gitdir reference.

As we do not need to handle fully qualfied paths we can make the
regex match 1 or more instead of 0 or more non-/ characters so use
's|[^/]\{1,\}|..|g' instead, which is correctly handled by all tested
sed implementations.  This expression is semantically different than
the original one.  It will not place leading '..' on a fully qualified
path as the original expression did.  None of the paths passed to the
regex relied on this behaviour so changing it shouldn't have negative
impact.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---

For some reason, I thought that using {x,y} range notation wasn't
valid in this scenario, but I think I was mistaken.  It seems to be
widely used elsewhere throughout the code.  I prefer that to the
[^/][^/]* notation and as they're equivalent, I switched to it.

 git-submodule.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index efc86ad..2c18e0c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,10 +167,11 @@ module_clone()
 	a=${a%/}
 	b=${b%/}
 
-	rel=$(echo $b | sed -e 's|[^/]*|..|g')
+	# Turn path components into .. 
+	rel=$(echo $b | sed -e 's|[^/]\{1,\}|..|g')
 	echo "gitdir: $rel/$a" >"$path/.git"
 
-	rel=$(echo $a | sed -e 's|[^/]*|..|g')
+	rel=$(echo $a | sed -e 's|[^/]\{1,\}|..|g')
 	(clear_local_git_env; cd "$path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
-- 
1.7.9

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> For some reason, I thought that using {x,y} range notation wasn't
> valid in this scenario, but I think I was mistaken.  It seems to be
> widely used elsewhere throughout the code.

Where?  Only two test scripts seem to use it and worse yet, one of them
uses it doubly incorrectly.

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

Excerpts from Junio C Hamano's message of Mon Apr 09 20:31:36 -0400 2012:

> > For some reason, I thought that using {x,y} range notation wasn't
> > valid in this scenario, but I think I was mistaken.  It seems to be
> > widely used elsewhere throughout the code.
> 
> Where?  Only two test scripts seem to use it and worse yet, one of them
> uses it doubly incorrectly.

I guess widely wasn't a good choice...My search wasn't narrow enough
earlier.  Was I correct in my original thinking that the range
notation isn't to be used then?

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> I guess widely wasn't a good choice...My search wasn't narrow enough
> earlier.  Was I correct in my original thinking that the range
> notation isn't to be used then?

So far we have tried to stay within a narrow subset of BRE (see
Documentation/CodingGuidelines) in fear of upsetting exotic and/or ancient
systems. I do not mind reviewing and revising the guidelines every couple
of years, but for this particular case, I do not think [^/][^/]* is too
bad.  FWIW, it bothers me a lot more that expression does not anchor
matches against path elements with explicit '/' than the issue your patch
addresses, i.e.

	sed -e 's|[^/][^/]*/|../|g'

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Andreas Schwab]

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

> FWIW, it bothers me a lot more that expression does not anchor
> matches against path elements with explicit '/' than the issue your patch
> addresses, i.e.
>
> 	sed -e 's|[^/][^/]*/|../|g'

Note that this requires readding a trailing slash to the path, which
complicates the whole thing a bit.  The effect of * has always been to
use the longest match.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> FWIW, it bothers me a lot more that expression does not anchor
>> matches against path elements with explicit '/' than the issue your patch
>> addresses, i.e.
>>
>> 	sed -e 's|[^/][^/]*/|../|g'
>
> Note that this requires readding a trailing slash to the path, which
> complicates the whole thing a bit.

Or simply swapping order of the stripping and sed invocation, which is not
a big deal.  I personally find the pattern with trailing '/' easier to grok
but it is also not a big deal.

Re: [PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

Excerpts from Junio C Hamano's message of Tue Apr 10 12:10:22 -0400 2012:
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> FWIW, it bothers me a lot more that expression does not anchor
> >> matches against path elements with explicit '/' than the issue your patch
> >> addresses, i.e.
> >>
> >>     sed -e 's|[^/][^/]*/|../|g'
> >
> > Note that this requires readding a trailing slash to the path, which
> > complicates the whole thing a bit.
> 
> Or simply swapping order of the stripping and sed invocation, which
> is not a big deal.  I personally find the pattern with trailing '/'
> easier to grok but it is also not a big deal.

Sorry for the delay here, I haven't had a chance to look at this in a
few days.  I tried the version with [^/][^/]*/ tonight and it was more
invasive than required for such a simple fix, I think.

Switching the order of the calls is ok, but then you need to use a
rela and relb as the second substitution relies on $a being
/-terminated and the creation of gitdir:... requires the opposite.  So
now, you're switching the order, moving other lines, renaming
variables and also altering the lines used to spit out the paths in
order to avoid also stripping the / from the rel* variables.

Updated patch to follow, but please don't hesitate to request the
alternate version if it really is what you'd like to see.  (I've done
the work already.)

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

[PATCH] Avoid bug in Solaris xpg4/sed as used in submodule

[Ben Walton]

The sed provided by Solaris in /usr/xpg4/bin has a bug whereby an
unanchored regex using * for zero or more repetitions sees two
separate matches fed to the substitution engine in some cases.

This is evidenced by:

$ for sed in /usr/xpg4/bin/sed /usr/bin/sed /opt/csw/gnu/sed; do \
echo 'ab' | $sed -e 's|[a]*|X|g'; \
done
XXbX
XbX
XbX

This bug was triggered during a git submodule clone operation as
exercised in the setup stage of t5526-fetch-submodules when using the
default SANE_TOOL_PATH for Solaris.  It led to paths such as
..../.. being used in the submodule .git gitdir reference.

As we do not need to handle fully qualfied paths we can make the regex
match 1 or more instead of 0 or more non-/ characters so use
's|[^/][^/]*|..|g' instead, which is correctly handled by all tested
sed implementations.  This expression is semantically different than
the original one.  It will not place leading '..' on a fully qualified
path as the original expression did.  None of the paths passed to the
regex relied on this behaviour so changing it shouldn't have negative
impact.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 git-submodule.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index efc86ad..7aa9e95 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -167,10 +167,10 @@ module_clone()
 	a=${a%/}
 	b=${b%/}
 
-	rel=$(echo $b | sed -e 's|[^/]*|..|g')
+	rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
 	echo "gitdir: $rel/$a" >"$path/.git"
 
-	rel=$(echo $a | sed -e 's|[^/]*|..|g')
+	rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
 	(clear_local_git_env; cd "$path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
-- 
1.7.9