patchAvoid bug in Solaris xpg4/sed as used in submodule

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(-)

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

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

Andreas.

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

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

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

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

Andreas.

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

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.

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

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

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(-)

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

s/Solairs/Solaris/

Andreas.

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

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.

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(-)

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

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

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

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

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

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'

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

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> writes:

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.

Excerpts from Junio C Hamano's message of Tue Apr 10 12:10:22 -0400 2012:

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

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(-)