Make the seccomp sandbox compatible with tcmalloc:...

Fix a few more places where we need to use our own allocator.
Make tcmalloc compatible with the seccomp sandbox by avoiding making direct system calls from within tcmalloc.

BUG=38973
TEST=none

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

[Evan Martin, 2010-03-24 19:25:36]

LGTM (as always, I don't fully understand it)

[agl, 2010-03-24 19:31:59]

LGTM

http://codereview.chromium.org/1294001/diff/1/4
File sandbox/linux/seccomp/library.h (right):

http://codereview.chromium.org/1294001/diff/1/4#newcode152
sandbox/linux/seccomp/library.h:152: typedef std::map<Elf_Addr, Range,
GreaterThan,
Any reason why this is GreaterThan and all the rest are <?

[willchan no longer on Chromium, 2010-03-24 19:39:59]

I'm going to update your changelist description to be a bit more exact if you
don't mind.  Otherwise, LGTM mod the one comment below.

http://codereview.chromium.org/1294001/diff/1/2
File third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h (right):

http://codereview.chromium.org/1294001/diff/1/2#newcode51
third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:51:
syscall(__NR_futex, &x, FUTEX_WAKE, 1, 0) >= 0);
Please document why we have this difference from upstream, so in the future when
we merge changes from upstream, we don't lose this.  Note why it cannot be
upstreamed (since upstream needs to work with more versions of glibc for which
this might not work correctly).

[willchan no longer on Chromium, 2010-03-24 19:42:15]

http://codereview.chromium.org/1294001/diff/1/4
File sandbox/linux/seccomp/library.h (right):

http://codereview.chromium.org/1294001/diff/1/4#newcode155
sandbox/linux/seccomp/library.h:155: typedef std::map<string, std::pair<int,
Elf_Shdr>, std::less<string>,
you need #include <functional> for std::less too.

[Markus (顧孟勤), 2010-03-25 20:17:53]

I added a brief comment.

In short, it has to do with the fact that we want to be able to use
lower_bound() to search through memory ranges, but it doesn't really have a
symmetric equivalent for both sorting orders. The somewhat confusingly named
upper_bound() is not actually the thing we need.

But creating the container with a custom comparator addresses the problem.

On 2010/03/24 19:31:59, agl wrote:
> LGTM
> 
> http://codereview.chromium.org/1294001/diff/1/4
> File sandbox/linux/seccomp/library.h (right):
> 
> http://codereview.chromium.org/1294001/diff/1/4#newcode152
> sandbox/linux/seccomp/library.h:152: typedef std::map<Elf_Addr, Range,
> GreaterThan,
> Any reason why this is GreaterThan and all the rest are <?

[agl, 2010-03-25 20:18:53]

On Thu, Mar 25, 2010 at 4:17 PM,  <markus@chromium.org> wrote:
>> LGTM

LGTM still applies in case you were waiting.


AGL

[Markus (顧孟勤), 2010-03-25 20:19:31]

Sure. I don't mind you changing the change description. Technically, we are
fixing a bug that was triggered by tcmalloc. But the bug was clearly in the
sandboxing code and not in tcmalloc. It's just that the system default malloc
didn't happen to ever trigger it.

As for the change in spinlock header, I now added a comment that should explain
the rationale for this change.

On 2010/03/24 19:39:59, willchan wrote:
> I'm going to update your changelist description to be a bit more exact if you
> don't mind.  Otherwise, LGTM mod the one comment below.
> 
> http://codereview.chromium.org/1294001/diff/1/2
> File third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h (right):
> 
> http://codereview.chromium.org/1294001/diff/1/2#newcode51
> third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:51:
> syscall(__NR_futex, &x, FUTEX_WAKE, 1, 0) >= 0);
> Please document why we have this difference from upstream, so in the future
when
> we merge changes from upstream, we don't lose this.  Note why it cannot be
> upstreamed (since upstream needs to work with more versions of glibc for which
> this might not work correctly).

[Markus (顧孟勤), 2010-03-25 20:19:59]

On 2010/03/24 19:42:15, willchan wrote:
> http://codereview.chromium.org/1294001/diff/1/4
> File sandbox/linux/seccomp/library.h (right):
> 
> http://codereview.chromium.org/1294001/diff/1/4#newcode155
> sandbox/linux/seccomp/library.h:155: typedef std::map<string, std::pair<int,
> Elf_Shdr>, std::less<string>,
> you need #include <functional> for std::less too.

Done (and thanks for spotting that)

[willchan no longer on Chromium, 2010-03-25 20:28:15]

LGTM, thanks for the updates!

[Markus (顧孟勤), 2013-01-25 22:24:00]

https://codereview.chromium.org/1294001/diff/9001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h (right):

https://codereview.chromium.org/1294001/diff/9001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/base/spinlock_linux-inl.h:90: nanosleep(&tm,
NULL);
I was researching some unrelated problem, and came across
http://stackoverflow.com/questions/7086220/what-does-rep-nop-mean-in-x86-asse...

It sounds as if our current code can wreak havoc with hyper-threading. We should
probably insert an:

  asm volatile("rep nop");

We might also need this instruction for the syscall(__NR_futex); not entirely
sure if it is needed there though. The system call might already do this.

We should probably run this past somebody who knows more about CPU
architectures? Maybe, m3b?