code review 9235046: runtime: do not mark os memory as executable on windows

runtime: do not mark os memory as executable on windows

[brainman, 2013-05-17 02:54:00]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-05-17 03:17:49]

LGTM



On Thu, May 16, 2013 at 7:54 PM, <alex.brainman@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> runtime: do not mark os memory as executable on windows
>
> Please review this at
https://codereview.appspot.**com/9235046/<https://codereview.appspot.com/9235...
>
> Affected files:
>   M src/pkg/runtime/mem_windows.c
>
>
> Index: src/pkg/runtime/mem_windows.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/mem_windows.**c
> +++ b/src/pkg/runtime/mem_windows.**c
> @@ -13,6 +13,7 @@
>         MEM_RESERVE = 0x2000,
>         MEM_RELEASE = 0x8000,
>
> +       PAGE_READWRITE = 0x0004,
>         PAGE_EXECUTE_READWRITE = 0x40,
>  };
>
> @@ -25,7 +26,7 @@
>  runtime·SysAlloc(uintptr n)
>  {
>         mstats.sys += n;
> -       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
> (uintptr)(MEM_COMMIT|MEM_**RESERVE), (uintptr)PAGE_EXECUTE_**READWRITE);
> +       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
> (uintptr)(MEM_COMMIT|MEM_**RESERVE), (uintptr)PAGE_READWRITE);
>  }
>
>  void
> @@ -51,12 +52,12 @@
>  {
>         // v is just a hint.
>         // First try at v.
> -       v = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
> (uintptr)MEM_RESERVE, (uintptr)PAGE_EXECUTE_**READWRITE);
> +       v = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
> (uintptr)MEM_RESERVE, (uintptr)PAGE_READWRITE);
>         if(v != nil)
>                 return v;
>
>         // Next let the kernel choose the address.
> -       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
> (uintptr)MEM_RESERVE, (uintptr)PAGE_EXECUTE_**READWRITE);
> +       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
> (uintptr)MEM_RESERVE, (uintptr)PAGE_READWRITE);
>  }
>
>  void
> @@ -65,7 +66,7 @@
>         void *p;
>
>         mstats.sys += n;
> -       p = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
> (uintptr)MEM_COMMIT, (uintptr)PAGE_EXECUTE_**READWRITE);
> +       p = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
> (uintptr)MEM_COMMIT, (uintptr)PAGE_READWRITE);
>         if(p != v)
>                 runtime·throw("runtime: cannot map pages in arena address
> space");
>  }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[khr1, 2013-05-17 03:26:17]

Is it worth calling GetSystemDEPPolicy/SetProcessDEPPolicy to make sure we
get it even if it is off for the host OS?  Or is it on by default for all
Windowsen we care about?


On Thu, May 16, 2013 at 8:17 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
>
> On Thu, May 16, 2013 at 7:54 PM, <alex.brainman@gmail.com> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> runtime: do not mark os memory as executable on windows
>>
>> Please review this at
https://codereview.appspot.**com/9235046/<https://codereview.appspot.com/9235...
>>
>> Affected files:
>>   M src/pkg/runtime/mem_windows.c
>>
>>
>> Index: src/pkg/runtime/mem_windows.c
>> ==============================**==============================**=======
>> --- a/src/pkg/runtime/mem_windows.**c
>> +++ b/src/pkg/runtime/mem_windows.**c
>> @@ -13,6 +13,7 @@
>>         MEM_RESERVE = 0x2000,
>>         MEM_RELEASE = 0x8000,
>>
>> +       PAGE_READWRITE = 0x0004,
>>         PAGE_EXECUTE_READWRITE = 0x40,
>>  };
>>
>> @@ -25,7 +26,7 @@
>>  runtime·SysAlloc(uintptr n)
>>  {
>>         mstats.sys += n;
>> -       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
>> (uintptr)(MEM_COMMIT|MEM_**RESERVE), (uintptr)PAGE_EXECUTE_**READWRITE);
>> +       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
>> (uintptr)(MEM_COMMIT|MEM_**RESERVE), (uintptr)PAGE_READWRITE);
>>  }
>>
>>  void
>> @@ -51,12 +52,12 @@
>>  {
>>         // v is just a hint.
>>         // First try at v.
>> -       v = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
>> (uintptr)MEM_RESERVE, (uintptr)PAGE_EXECUTE_**READWRITE);
>> +       v = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
>> (uintptr)MEM_RESERVE, (uintptr)PAGE_READWRITE);
>>         if(v != nil)
>>                 return v;
>>
>>         // Next let the kernel choose the address.
>> -       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
>> (uintptr)MEM_RESERVE, (uintptr)PAGE_EXECUTE_**READWRITE);
>> +       return runtime·stdcall(runtime·**VirtualAlloc, 4, nil, n,
>> (uintptr)MEM_RESERVE, (uintptr)PAGE_READWRITE);
>>  }
>>
>>  void
>> @@ -65,7 +66,7 @@
>>         void *p;
>>
>>         mstats.sys += n;
>> -       p = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
>> (uintptr)MEM_COMMIT, (uintptr)PAGE_EXECUTE_**READWRITE);
>> +       p = runtime·stdcall(runtime·**VirtualAlloc, 4, v, n,
>> (uintptr)MEM_COMMIT, (uintptr)PAGE_READWRITE);
>>         if(p != v)
>>                 runtime·throw("runtime: cannot map pages in arena address
>> space");
>>  }
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>> .
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>
>  --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[brainman, 2013-05-17 03:37:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=3c2cddfbdaec ***

runtime: do not mark os memory as executable on windows

R=golang-dev, bradfitz, khr
CC=golang-dev
https://codereview.appspot.com/9235046

[brainman, 2013-05-17 03:38:06]

On 2013/05/17 03:26:17, khr1 wrote:
> Is it worth calling GetSystemDEPPolicy/SetProcessDEPPolicy to make sure we
> get it even if it is off for the host OS? ...

I am not Windows expert - I don't know. I would leave it as is until one day we
have a real issue.

Alex

[dave_cheney.net, 2013-05-17 03:39:05]

Sounds reasonable to me. 

On 17/05/2013, at 13:38, alex.brainman@gmail.com wrote:

> On 2013/05/17 03:26:17, khr1 wrote:
>> Is it worth calling GetSystemDEPPolicy/SetProcessDEPPolicy to make
> sure we
>> get it even if it is off for the host OS? ...
> 
> I am not Windows expert - I don't know. I would leave it as is until one
> day we have a real issue.
> 
> Alex
> 
> https://codereview.appspot.com/9235046/
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
>