Landing: Write protect executable pages in the VM.

Landing: Write protect executable pages in the VM.

Change executable pages to be read/execute but not writable by default.

All pages are made temporarily writable just before a full GC, because both
the mark and sweep phases write to the pages. When allocating in a page and
when patching code, the pages are made temporarily writable.

The order of allocation of Code and Instructions objects is changed so that
a GC will not occur after Instructions is allocated. (A full GC would
render the Instructions unwritable.) A scoped object is used to make memory
protection simpler.

Original CL: https://codereview.chromium.org/106593002/

I added a cc test that is expected to crash.

[Florian Schneider, 2014-01-16 13:12:38]

This working on Windows, Linux. I will test Mac as well.

https://codereview.chromium.org/136563002/diff/1/runtime/vm/freelist.cc
File runtime/vm/freelist.cc (right):

https://codereview.chromium.org/136563002/diff/1/runtime/vm/freelist.cc#newco...
runtime/vm/freelist.cc:86: size + FreeListElement::kHeaderSize,
If the remainder is zero size, don't add kHeaderSize.

https://codereview.chromium.org/136563002/diff/1/runtime/vm/freelist.cc#newco...
runtime/vm/freelist.cc:107: size + FreeListElement::kHeaderSize,
If the remainder is zero size, don't add kHeaderSize.

https://codereview.chromium.org/136563002/diff/1/runtime/vm/virtual_memory_wi...
File runtime/vm/virtual_memory_win.cc (right):

https://codereview.chromium.org/136563002/diff/1/runtime/vm/virtual_memory_wi...
runtime/vm/virtual_memory_win.cc:89: return
VirtualProtect(reinterpret_cast<void*>(address),
s/address/page_address/

https://codereview.chromium.org/136563002/diff/1/runtime/vm/virtual_memory_wi...
runtime/vm/virtual_memory_win.cc:90: end_address - page_address,
Or s/page_address/address/.

[srdjan, 2014-01-16 17:38:12]

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/assembler.cc
File runtime/vm/assembler.cc (right):

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/assembler.cc...
runtime/vm/assembler.cc:160: intptr_t AssemblerBuffer::CountPointerOffsets()
const {
IMO, PointerOffsetsCount or NumberOfPointerOffsets sounds more descriptive of
the action.

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/code_patcher.h
File runtime/vm/code_patcher.h (right):

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/code_patcher...
runtime/vm/code_patcher.h:32: intptr_t size_;
Both can be const. Please add a brief comment about this class.

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_linux.cc (right):

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_linux.cc:16: // #include "vm/os.h"
Remove dead code.

[Florian Schneider, 2014-01-17 10:46:59]

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/code_patcher.h
File runtime/vm/code_patcher.h (right):

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/code_patcher...
runtime/vm/code_patcher.h:32: intptr_t size_;
On 2014/01/16 17:38:12, srdjan wrote:
> Both can be const. Please add a brief comment about this class.

Done.

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_linux.cc (right):

https://codereview.chromium.org/136563002/diff/160001/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_linux.cc:16: // #include "vm/os.h"
On 2014/01/16 17:38:12, srdjan wrote:
> Remove dead code.

Done.

[Florian Schneider, 2014-01-28 16:42:03]

Rebased and added a command line flag to control write protection.

[srdjan, 2014-01-31 17:24:14]

lgtm

[Ivan Posva, 2014-02-03 05:44:35]

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_mi...
File runtime/vm/assembler_mips.h (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_mi...
runtime/vm/assembler_mips.h:170: return buffer_.CountPointerOffsets();
Does MIPS code really contain pointer offsets?

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.h:765: return buffer_.CountPointerOffsets();
Does x64 really create pointer offsets or are all pointers loaded through the
pool pointer?

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc
File runtime/vm/freelist.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:68: VirtualMemory::kReadWriteExecute);
We should maintain either RW- or R-X. This state here should not be allowed.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:88: ((remainder_size > 0) ? FreeListElement::kHeaderSize
: 0);
remainder_size should always be non-zero if you are in this block of code.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:315: VirtualMemory::kReadExecute);
R-X protection assumes is_protected being equivalent to is_execute,  correct?

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.h#n...
runtime/vm/freelist.h:23: static const intptr_t kHeaderSize = 3 * kWordSize;
This does not look and sound right. Especially how are you adding two-word sized
objects to the free list.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:114: prot = VirtualMemory::kReadWriteExecute;
This should not be a legal state.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_android.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_android.cc:82: uword page_address = start_address &
~(PageSize() - 1);
How about using RoundDown?

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_linux.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_linux.cc:82: uword page_address = start_address &
~(PageSize() - 1);
ditto

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_macos.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_macos.cc:82: uword page_address = start_address &
~(PageSize() - 1);
ditto

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_win.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_win.cc:70: uword page_address = start_address &
~(PageSize() - 1);
ditto

[Florian Schneider, 2014-02-10 11:56:17]

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.h:765: return buffer_.CountPointerOffsets();
On 2014/02/03 05:44:35, Ivan Posva wrote:
> Does x64 really create pointer offsets or are all pointers loaded through the
> pool pointer?

Possibly yes, if CanLoadImmediateFromPool return false, there maybe immediates
in the generated code.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc
File runtime/vm/freelist.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:68: VirtualMemory::kReadWriteExecute);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> We should maintain either RW- or R-X. This state here should not be allowed.

Done. Changed rwx -> rw below, too.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:88: ((remainder_size > 0) ? FreeListElement::kHeaderSize
: 0);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> remainder_size should always be non-zero if you are in this block of code.

Why? In the case of hitting the region boundary it can be zero. This is the case
considered here. We must not cross page boundary here. 

VirtualProtect will crash otherwise: On Windows, the region to be protected has
to be within the same region that was allocated using VirtualAlloc.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.cc#...
runtime/vm/freelist.cc:315: VirtualMemory::kReadExecute);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> R-X protection assumes is_protected being equivalent to is_execute,  correct?

Yes.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.h
File runtime/vm/freelist.h (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/freelist.h#n...
runtime/vm/freelist.h:23: static const intptr_t kHeaderSize = 3 * kWordSize;
On 2014/02/03 05:44:35, Ivan Posva wrote:
> This does not look and sound right. Especially how are you adding two-word
sized
> objects to the free list.

This is a little confusing, but should be right: The header can be 2 or 3 words.

The header of the remainder element has to be unprotected for the
SplitAndEnqueue operation. It does not hurt to unprotect always 3 words here:
The remainder element will be protected in full again afterwards.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:114: prot = VirtualMemory::kReadWriteExecute;
On 2014/02/03 05:44:35, Ivan Posva wrote:
> This should not be a legal state.

Done. Should rwx then be removed completely from the code?

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_android.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_android.cc:82: uword page_address = start_address &
~(PageSize() - 1);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> How about using RoundDown?

Done.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_linux.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_linux.cc:82: uword page_address = start_address &
~(PageSize() - 1);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_macos.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_macos.cc:82: uword page_address = start_address &
~(PageSize() - 1);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
File runtime/vm/virtual_memory_win.cc (right):

https://codereview.chromium.org/136563002/diff/350035/runtime/vm/virtual_memo...
runtime/vm/virtual_memory_win.cc:70: uword page_address = start_address &
~(PageSize() - 1);
On 2014/02/03 05:44:35, Ivan Posva wrote:
> ditto

Done.

[Florian Schneider, 2014-02-10 12:18:29]

Committed patchset #11 manually as r32493 (presubmit successful).