Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] Makefile.SH: Fix quote() function to escape backslashes but not minus signs #14659

Closed
p5pRT opened this issue Apr 18, 2015 · 9 comments
Closed

Comments

@p5pRT
Copy link

p5pRT commented Apr 18, 2015

Migrated from rt.perl.org#124343 (status was 'resolved')

Searchable as RT124343$

@p5pRT
Copy link
Author

p5pRT commented Apr 18, 2015

From @ntyni

The intent of the scary regexp in quote() of Makefile.SH seems to be to
escape everything but the listed characters, with the list including the
minus sign ('-') and the forward slash ('/'). As these usually need to
be escaped in regexps, they are preceded by backslashes ('\-\/').

However, a sed peculiarity seems to be that neither the backslash ('\')
(with some exceptions) nor the delimiter character is special inside a
character list with square brackets ('[LIST]' or '[^LIST]').
This can be seen with for example
echo 'f/o\o' | sed 's/[\/]//g'
which outputs 'foo'. Therefore the character list '[\-\/]' is interpreted
as a range of backslash to backslash, plus the forward slash, and is
equivalent to the shorter list '[\/]'.

The backslash case is appropriately documented in the GNU sed manual,
but I haven't found much documentation about the delimiter character
being allowed inside a character list. However, at least both GNU and
Solaris 2.9 sed programs exhibit this behaviour in my tests.

The attached proposed patch changes the list of allowed characters to
include the minus sign and the forward slash but exclude the backslash,
which is presumably the intended behaviour. The effect of the patch
can be seen by building in a directory with a minus sign in its name,
calling Configure with the -Duseshrplib argument, and grepping for
LD_LIBRARY_PATH in the resulting Makefile. Without the patch, the minus
sign will be escaped unnecessarily.

Builds in a directory with a backslash in its name still don't work,
first failing inside cpan/Archive-Tar, but at least we try a bit better
with this.

Thanks for your work on Perl,
--
Niko Tyni ntyni@​debian.org

@p5pRT
Copy link
Author

p5pRT commented Apr 18, 2015

From @ntyni

0001-Fix-quote-function-to-escape-backslashes-but-not-min.patch
From 361cd793dff23c00917ea82dbdd9b6d46d1d77d9 Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Sat, 18 Apr 2015 18:59:07 +0300
Subject: [PATCH] Fix quote() function to escape backslashes but not minus
 signs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The delimiter character isn't special in character square brackets,
and neither is the backslash. So '\-\' means just a range of backslash
to backslash, and the minus sign isn't included at all.

Substitution tested with GNU Solaris 9 sed programs.

Originally noticed by Kristoffer Grundström.

Bug-Debian: https://bugs.debian.org/754057
---
 Makefile.SH | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.SH b/Makefile.SH
index eb082c2..82fbc80 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -5,7 +5,7 @@
 quote() {
 	case "$1" in
 	'') echo "''" ;;
-	*)  echo "$1" | sed 's/\([^a-zA-Z0-9.:_\-\/]\)/\\\1/g' ;;
+	*)  echo "$1" | sed 's/\([^a-zA-Z0-9.:_/-]\)/\\\1/g' ;;
 	esac
 }
 
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2015

From @tonycoz

On Sat Apr 18 11​:38​:57 2015, ntyni@​debian.org wrote​:

The attached proposed patch changes the list of allowed characters to
include the minus sign and the forward slash but exclude the backslash,
which is presumably the intended behaviour. The effect of the patch
can be seen by building in a directory with a minus sign in its name,
calling Configure with the -Duseshrplib argument, and grepping for
LD_LIBRARY_PATH in the resulting Makefile. Without the patch, the minus
sign will be escaped unnecessarily.

I also tested this on NetBSD and HP-UX.

According to​:

http​://pubs.opengroup.org/onlinepubs/009695399/utilities/sed.html#tag_04_126_13_02

the s/// BRE treats escaped delimiters as a literal delimiter, and doesn't mention any special behaviour in brackets.

But it behaves are you described (HP-UX)​:

bash-4.3$ echo 'foo bar/\' | sed 's/\([^a-zA-Z0-9.​:_/-]\)/\\\1/g'
foo\ bar/\\
bash-4.3$ echo 'foo bar/\' | sed 's/\([^a-zA-Z0-9.​:_\/-]\)/\\\1/g'
foo\ bar/\

I've added this to my post-5.22 branch.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2015

From @tonycoz

On Sat Apr 18 11​:38​:57 2015, ntyni@​debian.org wrote​:

The attached proposed patch changes the list of allowed characters to
include the minus sign and the forward slash but exclude the backslash,
which is presumably the intended behaviour. The effect of the patch
can be seen by building in a directory with a minus sign in its name,
calling Configure with the -Duseshrplib argument, and grepping for
LD_LIBRARY_PATH in the resulting Makefile. Without the patch, the minus
sign will be escaped unnecessarily.

Builds in a directory with a backslash in its name still don't work,
first failing inside cpan/Archive-Tar, but at least we try a bit better
with this.

Thanks for your work on Perl,

Thanks, applied as 87d9837.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2015

@tonycoz - Status changed from 'open' to 'resolved'

@p5pRT
Copy link
Author

p5pRT commented Jun 4, 2015

@tonycoz - Status changed from 'resolved' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant