Routines in our upper file layers can't handle ".." or "symlinks" well (and a...

Fix up ExtensionResource::CombinePathsSafely so that it can handle paths with .. in them safely.

BUG=25131
TEST=Build unit_tests on Mac with default build location that contains a .. in the path.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29383

[dmac, 2009-10-17 17:31:21]

Mark, pink:

Please let me know if you think this is right.

[Mark Mentovai, 2009-10-17 18:18:10]

dmac, thanks a bunch for this.  I suspected that we needed to do this, but the
only bug report I saw was by e-mail, and when I asked for more information, I
got no response.

http://codereview.chromium.org/295001/diff/1/2
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/295001/diff/1/2#newcode45
Line 45: // how the app was launched. We need to clean those up (and any
symlinks)
personal pet peeve: reword comments to not use "we."  This is fine if you just
say "clean those up..."

http://codereview.chromium.org/295001/diff/1/2#newcode50
Line 50: char* executable_path =
Don't do this.  The existing code used WriteInto, please keep that.

http://codereview.chromium.org/295001/diff/1/2#newcode59
Line 59: NSString* executable_string =
This routine is somewhat critical in that we call it repeatedly and it may have
implications on startup time.  The operations here aren't necessarily
time-consuming, but still, given the work that's being added, it makes sense to
only do this once and save it in a static.

Problem: we're not positive of thread safety here, so we can't use static.

Solution: use a base::Singleton.

I'm not going to force you into a singleton now: if you get rid of the
malloc/free and switch back to WriteInto, I think we're fine, and you or I can
do a followup change to refactor this into a singleton.

[Mark Mentovai, 2009-10-17 18:24:16]

(I had more to say, but I got a phone call and hit the wrong thing.)

http://codereview.chromium.org/295001/diff/1/2
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/295001/diff/1/2#newcode51
Line 51: static_cast<char*>(malloc(executable_length + 1));
Also, the value that we got back from the first _NSGetExecutablePath already has
room for the trailing NUL byte.  The |+ 1| is extraneous.

If you take a look at WriteInto in base/string_util.h, you'll see that it also
accepts a length parameter that includes space for the trailing NUL.

http://codereview.chromium.org/295001/diff/1/2#newcode58
Line 58: executable_path[executable_length] = 0;
...and _NSGetExecutablePath already NUL-terminates, so this isn't necessary...

...and, not that it matters here because it's not necessary, but it's advisable
to use '\0' when you want a NUL character literal, to make it clearer.

[Mark Mentovai, 2009-10-17 18:46:56]

Sorry, one more higher-level comment.

Given what -[NSString stringByStandardizingPath:] is capable of doing, I think
that this approach might be a little heavy-handed.  It's capable of doing a lot
of path transformations that aren't necessarily appropriate on on paths returned
by _NSGetExecutablePath(), and it might perform FS consultation.

For example, stringByStandardizingPath: can call stringByExpandingTildeInPath:,
which isn't right for these paths.  In the event that _NSGetExecutablePath()
does return a string with a leading ~, it won't be a home directory reference,
it'll be a reference to something in the filesystem whose name begins with ~. 
stringByExpandingTildeInPath: would be wrong.

The question now becomes what transformations we want to apply to the path that
_NSGetExecutablePath() returns.  I think that it's probably sufficient to make
sure that it's absolute, even if it does contain .s and ..s and symbolic links. 
If I'm off base here, please speak up!  Specifically, you've called out .. and
symbolic link references as bad.  Is that what's causing your unit tests to
fail?

I suspect that it would be both sufficient and actually more correct for our
purposes if we checked IsAbsolute() on the path returned by
_NSGetExecutablePath().  If the path is not absolute, then we should append it
to DIR_CURRENT.  Given that we (1) should be calling this early before anyone's
changed directory and (2) shouldn't really be changing directory anyway, I think
that this is fine.

Also, I looked more closely at the implementation of base/path_service.cc and
see that it already caches things, so I no longer think that doing the singleton
(or static) thing in here is correct.

[dmac, 2009-10-17 18:59:25]

The problem I was running into specifically was paths that contain '..' and
symlinks. Take a look specifically at file_path.h IsParent() which is called by
ExtensionResource::CombinePathsSafely, which is called by the resource loading
code which was causing my extension loading to fail.

Also, I don't see it documented anywhere that _NSGetExecutablePath has a
terminating 0. I can see that it would make sense that it does, but since it's
not documented I would feel safer putting the trailing terminator on there.

I would expect any path that contained ~ at its beginning would cause a whole
lot of things to fail (because a lot of the objective c file layer depends on
standardizing paths with that routine), but would you be happier if I used
realpath instead? It isn't documented as resolving ~, but does resolve .. and
symlinks. The only problem with realpath is trying to guess the size of the
buffer to pass in there. I can call it with a PATH_MAX buffer, but
_NSGetExecutablePath is explicitly documented as possibly returning something
with more that PATH_MAX chars. Given that the entire file system starts to keel
over and die once you have paths longer than PATH_MAX that may not be a problem,
but I'm open to other suggestions.

[dmac, 2009-10-18 04:38:17]

(Reposting a message from Mark that didn't get into the review)
Preface: Sorry to be such a pedantic pain about this.  I know that you
understand the issues at play here too.  I just want to make sure that
our solution is the least hand-wavey possible, and the one that allows
our app the best shot at running in even the weirdest corner cases.
Even if we have to accept a compromise, I want to make sure we
understand what we're giving up.  I don't mean to get in the way of
even checking in a temporary fix to get the unit tests working if
you'd like to do that, but I do think that we need to give this some
serious thought.

dmaclach@chromium.org wrote:
The problem I was running into specifically was paths that contain '..' and
symlinks. Take a look specifically at file_path.h IsParent() which is called by
ExtensionResource::CombinePathsSafely, which is called by the resource
loading
code which was causing my extension loading to fail.

Yup, OK.  Thanks for the additional info.  ".." is a can of worms and
FilePath isn't allowed to touch the disk, so...

If we need to do any fix-up in here, I guess we'll need it to hit the
disk after all.  OK, that's not the end of the world.  We'll only do
it once and I guess there can be benefits to getting this stuff all
resolved at once, up front, instead of having to hit the filesystem to
resolve it each time it's used.

(Could you guess that I'm always thinking of what a change like this
might do to startup performance?)

Also, I don't see it documented anywhere that _NSGetExecutablePath has a
terminating 0. I can see that it would make sense that it does, but since it's
not documented I would feel safer putting the trailing terminator on there.

Ah, but we've got dyld source and we can see that it has a terminating
NUL, and if anything ever fell into the "they can't change it because
it would break lots of stuff in a really bad way" category, I think
it'd be this.

I would expect any path that contained ~ at its beginning would cause a whole
lot of things to fail (because a lot of the objective c file layer depends on
standardizing paths with that routine),

That's true.  Maybe we should just CHECK that the path doesn't begin
with a ~, or maybe we could make them safe by prepending "./".

but would you be happier if I used
realpath instead? It isn't documented as resolving ~, but does resolve .. and
symlinks.

To be clear, I think we only care about ".."s but not symbolic links,
unless you've got another case to point to where symlinks might bite
us.

You point out the right bug in realpath:

The only problem with realpath is trying to guess the size of the
buffer to pass in there. I can call it with a PATH_MAX buffer, but
_NSGetExecutablePath is explicitly documented as possibly returning something
with more that PATH_MAX chars. Given that the entire file system starts to keel
over and die once you have paths longer than PATH_MAX that may not be a problem,
but I'm open to other suggestions.

It's actually a little bit worse: even if you give realpath a bigger
buffer, there's no way to communicate the buffer size to it; it always
just assumes you've given it PATH_MAX.  Yuck, what a terrible
interface.

I guess this comes down to two options that would be fixes in this file:

1. short-term, use realpath, and maybe medium-term, do a realpath
reimplementation using FilePaths or std::strings that doesn't have the
PATH_MAX fixed-size buffer problem.  Pro: the Mac implementation of
realpath always returns absolute paths, so a plugin or something else
changing directory won't break our use of paths resolved by this API.
Con: short-term, PATH_MAX bug, and long-term, more local code (and
it's somewhat tricky code).  Con: realpath wants to make everything
absolute, so even if we solve the PATH_MAX problem for ourselves, code
outside of our control will choke on these long absolute paths.

or

2. use stringByStandardizingPath with a CHECK or a fix-up for paths
that we know will be trouble, like those that begin with a ~.  The
fix-up could be "if it starts with ~, prepend ./".  Pro: in my tests,
stringByStandardizingPath is at least four times faster than realpath.
Con: stringByStandardizingPath is still a bit of a black box.  Con:
it seems like you really want to resolve ".."s in pathnames, but
stringByStandardizingPath calls stringByResolvingSymlinksInPath, which
is a no-op on relative paths and thus still doesn't fully solve the
problem.

If we go this way, I have one more suggestion: -[NSFileManager
stringWithFileSystemRepresentation:length:] instead of [NSString
stringWithUTF8String:]?

Both of these options suck.  If there's any clear winner, it's "deal
with what the system gives us as best as possible and with as few
fix-ups as possible" because it's the least likely to break under
PATH_MAX stress, and when it does break, other applications would be
likely to break under the same conditions.  I don't want to paint
ourselves into a corner where we're the only application that's
intolerant of a weird corner case.  I'm willing to buy that
stringByStandardizingPath with a CHECK or fix-up is a viable
workaround for now.  At least in the normal user case, we don't expect
".."s to come back from _NSGetExecutablePath.

I'll advance another option:

3. How are you invoking the unit tests?  Based on your description, it
sounds like you're doing it from the convenience targets in the
generated Xcode projects?  If that's it, we can just make those
convenience targets resolve dots before starting the tests.  If we're
not going to expect dots from _NSGetExecutablePath in real user
environments, this might be a better workaround for now.  Pro: least
likely to break under PATH_MAX stress.  Con: still broken when the
application is launched with dots in the path, such as from the
command line; still broken for unit tests launched with dots by
something other than the Xcode convenience targets.  Con: I still said
"for now" for this one.  :)

but

It kind of bugs me that we need to do this at all.  Doing these
fix-ups wouldn't be so bad if there were a fix-up option that didn't
suck.  Maybe this really is the wrong layer to be attacking.  I can't
imagine that there are very many users of paths that actually care if
they're absolute or relative or have symbolic links or dots in them.
That brings us to:

4. It really looks to me like the
I-can't-resolve-dots-because-I-can't-ask-the-disk FilePath::IsParent
is absolutely the wrong thing for
ExtensionResource::CombinePathsSafely to be using.  In fact, that
whole function looks - I'll say it - wrong.  The dot issue makes
IsParent fragile, but IsParent comes with a warning about dots, and
CombinePathsSafely totally ignores the warning and does some really
weird-looking stuff for questionable reasons.  If there's a bug here,
I'd say it's in that function.  The maybe-they've-got-dots paths that
come out of base_paths_mac.mm aren't necessarily the only paths that
this method is going to see, and patching up base_paths_mac.mm might
just be papering over a problem that still might occur with other
paths.  Pro: least likely to break under PATH_MAX stress.  Con: none?

Is there anything else that cares about dots or symlinks, or is this
the only one you hit?

Mark

[dmac, 2009-10-18 06:10:06]

> 4. It really looks to me like the
> I-can't-resolve-dots-because-I-can't-ask-the-disk FilePath::IsParent
> is absolutely the wrong thing for
> ExtensionResource::CombinePathsSafely to be using.  In fact, that
> whole function looks - I'll say it - wrong.  The dot issue makes
> IsParent fragile, but IsParent comes with a warning about dots, and
> CombinePathsSafely totally ignores the warning and does some really
> weird-looking stuff for questionable reasons.  If there's a bug here,
> I'd say it's in that function.  The maybe-they've-got-dots paths that
> come out of base_paths_mac.mm aren't necessarily the only paths that
> this method is going to see, and patching up base_paths_mac.mm might
> just be papering over a problem that still might occur with other
> paths.  Pro: least likely to break under PATH_MAX stress.  Con: none?

Ok.. new version up that attempts to remedy #4 somewhat. I'm not sure why they
aren't just using "resolve" instead of "ReplaceComponents", but this change
fixes the problem, and keeps the changes to a minimum.

[Mark Mentovai, 2009-10-18 16:14:45]

1. Update the CL description.

2. LGTM

I'm not sure that the URL operations do the right thing with .. in file: URLs,
but that would be a more general problem in our file: URL handling code, and
with this change, we'd at least be behaving consistently.  It's actually not
entirely clear what the correct way to handle .. in a file: URL is.

[brettw, 2009-10-19 03:38:28]

I'm not going to block this CL, but I'm still uncomfortable using GURL for local
file operations. 

I think there's an old internal bug about GURL *not* resolving .. and . like you
rely on in this changelist (I remember Peter was complaining we didn't send ".."
to his server which he relied on for something). Other browsers do not do this,
they only resolve .. when computing a relative URL on a page.