LXR doesn't properly escape file and directory names.

NEW
Assigned to

Status

Webtools
MXR
16 years ago
11 years ago

People

(Reporter: Gregory Leblanc, Assigned: Gregory Leblanc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
Created attachment 53003 [details] [diff] [review]
regexp escapes $path and $filename
(Assignee)

Comment 2

16 years ago
Created attachment 53004 [details] [diff] [review]
Much shorter/nicer patch, since those variables aren't used elsewhere

Comment 3

16 years ago
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
(Assignee)

Comment 4

16 years ago
Created attachment 53017 [details] [diff] [review]
With comments!

Comment 5

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

r=kiko

thanks
+
$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

Summary: LXR doesn't properly escape file and directory names. → LXR doesn't properly escape file and directory names.
Said regexp is:

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

Gerv
(Assignee)

Comment 8

16 years ago
Created attachment 54985 [details] [diff] [review]
Patch to escape regexps, using the "safe" method recommended by Gerv
(Assignee)

Comment 9

16 years ago
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.
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

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

we do need to escape all of those for /search?
Assignee: endico → gleblanc
QA Contact: timeless → lxr
You need to log in before you can comment on or make changes to this bug.