Add guard pages in front of platform allocations

Add guard pages in front of executable allocations

BUG=89247
Committed: http://code.google.com/p/v8/source/detail?r=8687

[Cris Neckar, 2011-07-14 19:11:59]

new patch for bleeding edge branch

[Vyacheslav Egorov (Chromium), 2011-07-15 11:59:14]

we are almost there I think

I am redirecting the last review (and landing) to Mads Ager as I am on vacation
next week.

http://codereview.chromium.org/7379004/diff/16/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/7379004/diff/16/src/platform.h#newcode212
src/platform.h:212: // Get the Alignment guaranteed by Allocate().
empty line between declaration and comment.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode384
src/spaces.cc:384: OS::Guard(mem, Page::kPageSize);
You have decreased kPagesPerChunk for all spaces but you only use it when you
allocate executable memory.

I think this is still a bit low level and you need to move one level up and put
guard page management there.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode386
src/spaces.cc:386: mem = static_cast<char*>(mem) + Page::kPageSize;
we use Address type instead of char* in such cases.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode414
src/spaces.cc:414: OS::Free(static_cast<char*>(mem) - guardsize, length +
guardsize);
we use Address type instead of char* in such cases.

http://codereview.chromium.org/7379004/diff/16/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7379004/diff/16/src/spaces.h#newcode650
src/spaces.h:650: static const int kPagesPerChunkLog2 = 5;
1 << kPagesPerChunkLog2 no longer equals to kPagesPerChunk

I find this disturbing.

[Cris Neckar, 2011-07-15 18:51:29]

http://codereview.chromium.org/7379004/diff/16/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/7379004/diff/16/src/platform.h#newcode212
src/platform.h:212: // Get the Alignment guaranteed by Allocate().
On 2011/07/15 11:59:14, Vyacheslav Egorov wrote:
> empty line between declaration and comment.

Done.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode384
src/spaces.cc:384: OS::Guard(mem, Page::kPageSize);
On 2011/07/15 11:59:14, Vyacheslav Egorov wrote:
> You have decreased kPagesPerChunk for all spaces but you only use it when you
> allocate executable memory.
> 
> I think this is still a bit low level and you need to move one level up and
put
> guard page management there.

Done.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode386
src/spaces.cc:386: mem = static_cast<char*>(mem) + Page::kPageSize;
On 2011/07/15 11:59:14, Vyacheslav Egorov wrote:
> we use Address type instead of char* in such cases.

Done.

http://codereview.chromium.org/7379004/diff/16/src/spaces.cc#newcode414
src/spaces.cc:414: OS::Free(static_cast<char*>(mem) - guardsize, length +
guardsize);
On 2011/07/15 11:59:14, Vyacheslav Egorov wrote:
> we use Address type instead of char* in such cases.

Done.

http://codereview.chromium.org/7379004/diff/16/src/spaces.h
File src/spaces.h (right):

http://codereview.chromium.org/7379004/diff/16/src/spaces.h#newcode650
src/spaces.h:650: static const int kPagesPerChunkLog2 = 5;
On 2011/07/15 11:59:14, Vyacheslav Egorov wrote:
> 1 << kPagesPerChunkLog2 no longer equals to kPagesPerChunk
> 
> I find this disturbing.


Never fear.. This isn't used anywhere so I got rid of it.

[Mads Ager (chromium), 2011-07-17 09:47:53]

The handling of the guard page is at two different levels now. At one level when
handling allocation and at another level when handling deallocation. We should
fix that. In particular I don't like that the address you get from
AllocateRawMemory is not the address you should give to FreeRawMemory when you
want to deallocate it.

http://codereview.chromium.org/7379004/diff/12005/src/platform-cygwin.cc
File src/platform-cygwin.cc (right):

http://codereview.chromium.org/7379004/diff/12005/src/platform-cygwin.cc#newc...
src/platform-cygwin.cc:170: mprotect(address, size, PROT_NONE);
Instead of having identical code in all of these platform files, please just add
this to platform-posix.cc. That will take care of freebsd, openbsd, linux,
macos, solaris and cygwin.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode411
src/spaces.cc:411: OS::Free(static_cast<char*>(mem) - guardsize, length +
guardsize);
This looks nasty to me. Doesn't this mean that to free executable memory you
have to pass in another address than the one you got from AllocateRawMemory? We
should avoid that.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode2692
src/spaces.cc:2692: if (size < requested) {
Shouldn't you add guardsize to requested before you get here? Otherwise you
might not have enough space for the guard page.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode2694
src/spaces.cc:2694: static_cast<Address>(mem) - guardsize, size + guardsize,
executable);
I'm getting confused here. Does this add up? FreeRawMemory will subtract
guardsize again, right?

[Cris Neckar, 2011-07-18 23:55:12]

http://codereview.chromium.org/7379004/diff/12005/src/platform-cygwin.cc
File src/platform-cygwin.cc (right):

http://codereview.chromium.org/7379004/diff/12005/src/platform-cygwin.cc#newc...
src/platform-cygwin.cc:170: mprotect(address, size, PROT_NONE);
On 2011/07/17 09:47:53, Mads Ager wrote:
> Instead of having identical code in all of these platform files, please just
add
> this to platform-posix.cc. That will take care of freebsd, openbsd, linux,
> macos, solaris and cygwin.

Done.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode411
src/spaces.cc:411: OS::Free(static_cast<char*>(mem) - guardsize, length +
guardsize);
On 2011/07/17 09:47:53, Mads Ager wrote:
> This looks nasty to me. Doesn't this mean that to free executable memory you
> have to pass in another address than the one you got from AllocateRawMemory?
We
> should avoid that.

Done.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode2692
src/spaces.cc:2692: if (size < requested) {
On 2011/07/17 09:47:53, Mads Ager wrote:
> Shouldn't you add guardsize to requested before you get here? Otherwise you
> might not have enough space for the guard page.

Done.

http://codereview.chromium.org/7379004/diff/12005/src/spaces.cc#newcode2694
src/spaces.cc:2694: static_cast<Address>(mem) - guardsize, size + guardsize,
executable);
On 2011/07/17 09:47:53, Mads Ager wrote:
> I'm getting confused here. Does this add up? FreeRawMemory will subtract
> guardsize again, right?

Yeah this was a mistake, I had initially planned to do it the way that you asked
and forgot to get rid of this.

Done.

[Cris Neckar, 2011-07-19 00:24:28]

Missed a header so I uploaded another patch

[Mads Ager (chromium), 2011-07-19 09:11:13]

Getting there.

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc#newcode518
src/spaces.cc:518: ASSERT(*allocated_pages >= kPagesPerChunk - 1);
Will this actually work? What if we lose a page due to alignment and lose a page
because of the guard page? We could do all the checking here in exactly the same
way that we did before. Then move the guarding to after the if below and
subtract one page from allocate_pages if EXECUTABLE? That would also remove the
"+/- guardsize" in FreeRawMemory below.

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc#newcode2701
src/spaces.cc:2701: static_cast<Address>(mem) - guardsize, size + guardsize,
executable);
This looks wrong. You should use mem and size directly as you got them from
AllocateRawMemory.

[Cris Neckar, 2011-07-19 18:35:32]

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc#newcode518
src/spaces.cc:518: ASSERT(*allocated_pages >= kPagesPerChunk - 1);
On 2011/07/19 09:11:14, Mads Ager wrote:
> Will this actually work? What if we lose a page due to alignment and lose a
page
> because of the guard page? We could do all the checking here in exactly the
same
> way that we did before. Then move the guarding to after the if below and
> subtract one page from allocate_pages if EXECUTABLE? That would also remove
the
> "+/- guardsize" in FreeRawMemory below.

Done.

http://codereview.chromium.org/7379004/diff/21002/src/spaces.cc#newcode2701
src/spaces.cc:2701: static_cast<Address>(mem) - guardsize, size + guardsize,
executable);
On 2011/07/19 09:11:14, Mads Ager wrote:
> This looks wrong. You should use mem and size directly as you got them from
> AllocateRawMemory.

Yep I am dumb. :)

[Mads Ager (chromium), 2011-07-20 07:24:16]

LGTM. There is one more off by one. I'll take care of that and land.

http://codereview.chromium.org/7379004/diff/27001/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7379004/diff/27001/src/spaces.cc#newcode513
src/spaces.cc:513: kPagesPerChunk - ((guardsize ? 1 : 0) + 1));
Now that you have move the fiddling with the chunk pointer and the chunk size to
after the conditions you should not change the conditions at all. I think it is
off by a page in the other direction now.