Avoid all library calls (including calls to destructors) after enabling the sandbox.

Avoid all library calls (including calls to destructors) after enabling the sandbox.

This ensures that the sandbox doesn't surprisingly fail, if the user instantiated
a particularly strick policy.

BUG=130662
TEST=make && ./demo32 && ./demo64

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141851

[Markus (顧孟勤), 2012-06-08 21:42:48]

I am trying to keep all policy out of the sandbox compiler. But we accidentally
required that all policies should allow heap manipulation (i.e.
mmap()/munmap()).

This change list fixes that assumption -- at the cost of us temporarily copying
data onto the stack. I know that variable-length arrays are generally
frowned-upon, but it seems justified in this particular case.

[jln (very slow on Chromium), 2012-06-12 19:16:23]

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:216: Program *program = new Program();
I would love if we didn't have to keep track (and delete() manually) a pointer.
Obviously we cannot used scoped_ptr here, but I think we could have "Program
program;" as before, but in an inside block {} that closes before we install the
policy (and after we copy the program to the stack obviously).

This would rely on the same C++ guarantee as the one we have for Errorcode in
the for() loop below, etc.

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:314: // stack-allocated array.
Please explain further that you're trying to make sure that the program vector
gets de-allocated from the heap before installing the filter. As-is, I don't
think it'll be clear to reviewers.

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:317: memcpy(bpf, &(*program)[0],
sizeof(bpf));
Would a static_cast of program be cleaner here ?

[Markus (顧孟勤), 2012-06-12 19:35:05]

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:216: Program *program = new Program();
On 2012/06/12 19:16:23, Julien Tinnes wrote:
> I would love if we didn't have to keep track (and delete() manually) a
pointer.
> Obviously we cannot used scoped_ptr here, but I think we could have "Program
> program;" as before, but in an inside block {} that closes before we install
the
> policy (and after we copy the program to the stack obviously).
> 
> This would rely on the same C++ guarantee as the one we have for Errorcode in
> the for() loop below, etc.

This is tricky, and I don't think I can do it without writing even crazier code.

The problem is that we eventually need to allocate the "bpf" array on the stack.
And when we create it, we still need access to "program".

This results in two mutually exclusive requirements for block scoping.

So, I think, explicit management of the heap allocated object is required. Feel
free to correct me, if you can see an alternative that eludes me.

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:314: // stack-allocated array.
On 2012/06/12 19:16:23, Julien Tinnes wrote:
> Please explain further that you're trying to make sure that the program vector
> gets de-allocated from the heap before installing the filter. As-is, I don't
> think it'll be clear to reviewers.

Sure, I can add a little more to the comment :-) This is subtle code, and more
comments are always helpful.

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:317: memcpy(bpf, &(*program)[0],
sizeof(bpf));
On 2012/06/12 19:16:23, Julien Tinnes wrote:
> Would a static_cast of program be cleaner here ?

I am not convinced a cast would actually do the right thing. I believe we have
to make sure we execute the vector::operator[](). Feel free to correct me, if
you can find a something in the STL documentation that says otherwise.

[jln (very slow on Chromium), 2012-06-12 20:22:51]

LGTM

(assuming that the review tool is getting really confused because the diff base
is not the same in the two iterations of the patch, i.e. that you didn't change
anything else than the comment in this revision).

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:216: Program *program = new Program();
On 2012/06/12 19:35:05, Markus (顧孟勤) wrote:
> On 2012/06/12 19:16:23, Julien Tinnes wrote:
> > I would love if we didn't have to keep track (and delete() manually) a
> pointer.
> > Obviously we cannot used scoped_ptr here, but I think we could have "Program
> > program;" as before, but in an inside block {} that closes before we install
> the
> > policy (and after we copy the program to the stack obviously).
> > 
> > This would rely on the same C++ guarantee as the one we have for Errorcode
in
> > the for() loop below, etc.
> 
> This is tricky, and I don't think I can do it without writing even crazier
code.
> 
> The problem is that we eventually need to allocate the "bpf" array on the
stack.
> And when we create it, we still need access to "program".
> 
> This results in two mutually exclusive requirements for block scoping.
> 
> So, I think, explicit management of the heap allocated object is required.
Feel
> free to correct me, if you can see an alternative that eludes me.

You're right, this becomes even more confusing.

https://chromiumcodereview.appspot.com/10535089/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:317: memcpy(bpf, &(*program)[0],
sizeof(bpf));
On 2012/06/12 19:35:05, Markus (顧孟勤) wrote:
> On 2012/06/12 19:16:23, Julien Tinnes wrote:
> > Would a static_cast of program be cleaner here ?
> 
> I am not convinced a cast would actually do the right thing. I believe we have
> to make sure we execute the vector::operator[](). Feel free to correct me, if
> you can find a something in the STL documentation that says otherwise.

The correct solution would be to use the new vector::data(). I presume all of
our toolchains already support it, but feel free to stick to the current tired
method.

[Chris Evans, 2012-06-12 20:26:25]

LGTM, one minor nit, do what you will with it.
I like the spirit of this CL :)

https://chromiumcodereview.appspot.com/10535089/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/10535089/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:216: if (!program) {
Nit: not sure you need this, the standard c++ runtime should throw upon an
allocation failure?