104139 - LXR doesn't properly escape file and directory names.

Status: RESOLVED WONTFIX

(Webtools Graveyard :: MXR, defect)

Description

[Gregory Leblanc]

Using the stock LXR code on a CVS module named, for example ORBit-C++ fails. 
The source page displays "
** Fatal: /\s*ORBit-C++\s*-\s*-*\s*/: nested *?+ in regexp at Local.pm line 248,
 line 1.
"
It just seems to be using the raw path and filename (with / stripped from the
front and end of the path, I'm not sure why that is), but it needs to be
escaped.  Here's a patch (thanks to Kiko and Dekar for writing the patch) that
works on the new LXR I've set up for GNOME.

Comment 1

[Gregory Leblanc]

Attachment: regexp escapes $path and $filename

Comment 2

[Gregory Leblanc]

Attachment: Much shorter/nicer patch, since those variables aren't used elsewhere

Comment 3

[Christian Reis]

Comment on attachment 53004 [details] [diff] [review]
Much shorter/nicer patch, since those variables aren't used elsewhere

r=kiko if you add a comment to the region showing what the hell we are doing

Comment 4

[Gregory Leblanc]

Attachment: With comments!

Comment 5

[Christian Reis]

Comment on attachment 53017 [details] [diff] [review]
With comments!

r=kiko

thanks

Comment 6

[Gervase Markham [:gerv]]

+
$path =~ s/(\W)/\\\1/g;


Why not:

+
$path =~ s/(\W)/$1/g;

? It's much easier to see what's going on. Also, do we need to escape all
non-alphanumerics (\W)? Elsewhere in the code we have a canned regexp which only
escapes meta-characters. This seems safer.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Said regexp is:

  $foo =~ s/([\[\$\\({}).+?\/])/\\$1/g;

Gerv

Comment 8

[Gregory Leblanc]

Attachment: Patch to escape regexps, using the "safe" method recommended by Gerv

Comment 9

[Gregory Leblanc]

OK, so the method in the latest patch seems to miss "-" (bug 38060), and "|"
(bug 99680, which may include other characters, I didn't look too closely).  We
need to get at least those two picked up by this regular expression.  
Gerv, could you elaborate on why "s/(\W)/$1/g" isn't a good solution (simpler
form of the original regular expression).  I can't quite get a handle on the
security concerns here, and it seems that escaping all non-alphanumeric
characters will ensure that we don't see this sort of a bug arrise again.

Comment 10

[Gervase Markham [:gerv]]

Are you certain that there is no character X, where X is non-alphanumeric, where
\X does not mean something special? 

I don't know why you need to escape "-" - this is not a special regexp
character. My canned regexp doesn't escape |, * and ^ because they aren't legal
filename characters in the first place. You are escaping filenames, right?

Gerv

Comment 11

[timeless]

ok, nm the [-<|>] stuff, packages-win must be very special magic.

we do need to escape all of those for /search?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

mxr is gone, mass closing.
https://searchfox.org/ is a much better alternative.