Be more restrictive when finding file names for libraries that need patching....

Be more restrictive when finding file names for libraries that need patching.
This avoids false positives if the directory name matches one of the well-known
library names (e.g. ld).

False positives not only result in a performance hit at startup, because we
are now trying to instrument libraries that don't actually contain any system
calls; but even worse than this, we could try to instrument system calls in
the sandboxing code itself. And those system calls are deliberately coded so
that they will not get rewritten.

Fortunately, none of this is a security problem. If we accidentally rewrite
system calls that weren't supposed to be rewritten, we will just crash on
startup.

TEST=the sandbox now works on the buildbots
BUG=36133
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39839

[Markus (顧孟勤), 2010-02-24 00:49:55]

The heuristic for finding the libraries that need patching was overly liberal. I
intentional made it liberal, as it is generally OK to scan more libraries at
startup. It usually just results in poor performance, as we disassembly
everything and then discover that there is not a single system call that need
rewriting.

But making the heuristics too strict is usually not a good idea, as it will
result in the sandbox failing to find system calls should the name of the
standard libraries change subtly.

The only exception to all of this is that we must never try to patch the actual
binary that includes the sandbox itself. The sandbox code makes a couple of
system calls that must never be rewritten.

Before this bug fix, we would erroneously try to rewrite all libraries
(including the sandbox), if the current directory included the characters "ld"
(as in "build"). This caused failures on the buildbots.

I now explicitly strip the directory name from libraries, and check that the
match occurs at the beginning of the filename.

Hopefully, this tighter heuristic doesn't turn out to be too tight. But I guess,
we'll find out, if some distribution uses non-standard names for its libraries.

[agl, 2010-02-24 01:27:53]

http://codereview.chromium.org/652188/diff/1/2
File sandbox/linux/seccomp/sandbox.cc (right):

http://codereview.chromium.org/652188/diff/1/2#newcode482
sandbox/linux/seccomp/sandbox.cc:482: for (const char *delim = " /"; *delim;
++delim) {
Why the space? It seems that you're finding the last / before the last space.
Maybe a typical value of mapping should be included in the comment to make it
clear why this is correct.

[Markus (顧孟勤), 2010-02-24 01:43:08]

http://codereview.chromium.org/652188/diff/5/1003
File sandbox/linux/seccomp/sandbox.cc (right):

http://codereview.chromium.org/652188/diff/5/1003#newcode484
sandbox/linux/seccomp/sandbox.cc:484: // 08:01 2289011 /lib/libc-2.7.so
Is this better? I hope this makes sense now.

[agl, 2010-02-24 01:58:15]

http://codereview.chromium.org/652188/diff/5/1003
File sandbox/linux/seccomp/sandbox.cc (right):

http://codereview.chromium.org/652188/diff/5/1003#newcode484
sandbox/linux/seccomp/sandbox.cc:484: // 08:01 2289011 /lib/libc-2.7.so
On 2010/02/24 01:43:08, Markus (顧孟勤) wrote:
> Is this better? I hope this makes sense now.

Oh, yea. I just had the path in mind. Makes sense now.

Now my only concern is that strrchr can return NULL if the given char isn't
found. I guess all these paths are absolute, so will start with a / if nothing
else. If you're happy that "[vdso]" or "" doesn't slip in here, then LGTM.

[Markus (顧孟勤), 2010-02-24 02:02:10]

http://codereview.chromium.org/652188/diff/5/1003
File sandbox/linux/seccomp/sandbox.cc (right):

http://codereview.chromium.org/652188/diff/5/1003#newcode484
sandbox/linux/seccomp/sandbox.cc:484: // 08:01 2289011 /lib/libc-2.7.so
It's explicitly supposed to be able to deal with strings that don't contain any
of the delimiters. If strrchr() returns NULL, we will leave "mapping" unchanged.
Did I miss anything?

[agl, 2010-02-24 02:04:22]

http://codereview.chromium.org/652188/diff/5/1003
File sandbox/linux/seccomp/sandbox.cc (right):

http://codereview.chromium.org/652188/diff/5/1003#newcode484
sandbox/linux/seccomp/sandbox.cc:484: // 08:01 2289011 /lib/libc-2.7.so
On 2010/02/24 02:02:11, Markus (顧孟勤) wrote:
> It's explicitly supposed to be able to deal with strings that don't contain
any
> of the delimiters. If strrchr() returns NULL, we will leave "mapping"
unchanged.
> Did I miss anything?

No I'm just a muppet. You should commit this before I 'review' it any more :)