Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7648002:
Modify the NaCl_page_alloc_hint function to use VirtualQuery to check (Closed)
|
Created:
9 years, 4 months ago by bbudge Modified:
9 years, 4 months ago Reviewers:
Brad Chen, cpu_(ooo_6.6-7.5), commit-bot: I haz the power, bsy_cr, Mark Seaborn, Erik does not do reviews CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionModify sel_memory functions to support pre-reserved sandbox memory on 32 bit
platforms. Add a NaCl_find_sandbox_memory function to use VirtualQuery on
Windows to check if the parent process has already reserved a 1Gb memory region.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2131
TEST=manual
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6451
Patch Set 1 #
Total comments: 3
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 24
Patch Set 4 : '' #
Total comments: 5
Patch Set 5 : '' #
Messages
Total messages: 13 (0 generated)
http://codereview.chromium.org/7648002/diff/1/src/trusted/service_runtime/win... File src/trusted/service_runtime/win/sel_memory.c (right): http://codereview.chromium.org/7648002/diff/1/src/trusted/service_runtime/win... src/trusted/service_runtime/win/sel_memory.c:113: if (num_bytes == kOneGb) { Having a special case for 1GB in NaCl_page_alloc() seems hacky, because this function is used for purposes other than allocating address space. I'd add a case to NaClAllocateSpace() in sel_addrspace_x86_32.c instead, which already has an #if for the related zero-based-sandbox case. http://codereview.chromium.org/7648002/diff/1/src/trusted/service_runtime/win... src/trusted/service_runtime/win/sel_memory.c:116: { Style: '{' should be on previous line http://codereview.chromium.org/7648002/diff/1/src/trusted/service_runtime/win... src/trusted/service_runtime/win/sel_memory.c:118: { Ditto
I'm trying an approach suggested by Mark, where we use a Windows quirk to poke the reserved memory address into the sel_ldr process. However, I can't get it to work. I'm going to run the idea by Carlos and other Windows experts, but in the meantime, since we need a fix soon, I'm addressing Mark's comments on the original CL. PTAL
http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:14: const size_t kOneGb = 0x40000000; I'm used to 'b' meaning 'bit'. I'd rather see kOneGB here. Some folks find (1 << 30) more readable than 0x40000000; your call. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: result = NaCl_page_alloc_at_addr(mem, addrsp_size); I don't understand how mem gets initialized in this case.
http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: result = NaCl_page_alloc_at_addr(mem, addrsp_size); On 2011/08/16 01:21:49, Brad Chen wrote: > I don't understand how mem gets initialized in this case. It's obscure, but the 'mem' parameter gets filled in by the call to NaCl_find_sandbox_memory if the pre-reserved region is found. Then the call to NaCl_page_alloc_at_addr tries to use that address.
http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: result = NaCl_page_alloc_at_addr(mem, addrsp_size); Nit: A brief comment before the call to NaCl_page_alloc_at_addr might be helpful. On 2011/08/16 17:21:02, bbudge1 wrote: > On 2011/08/16 01:21:49, Brad Chen wrote: > > I don't understand how mem gets initialized in this case. > > It's obscure, but the 'mem' parameter gets filled in by the call to > NaCl_find_sandbox_memory if the pre-reserved region is found. Then the call to > NaCl_page_alloc_at_addr tries to use that address.
Otherwise LGTM. Please check with Mark in case he has any other feedback.
The new organisation looks good. Some nits... http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:29: #elif NACL_WINDOWS && NACL_ARCH_CPU_32_BITS Can you use "NACL_BUILD_SUBARCH == 64" rather than "NACL_ARCH_CPU_32_BITS", for consistency? No-one is using the latter. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:35: if (0 == NaCl_find_sandbox_memory(mem, kOneGb)) Nit: I'd use curly brackets for this 'if'. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/sel_memory.h (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/sel_memory.h:30: int NaCl_find_sandbox_memory(void **p, Maybe name it something like "NaCl_find_prereserved_sandbox_memory()", to make it a little clearer? http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/win/sel_memory.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:27: #if NACL_ARCH_CPU_32_BITS As before. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:29: int NaCl_find_sandbox_memory(void **p, Can you put a comment in to say that the purpose of this is to look for a region reserved by the parent process on our behalf? http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:33: char* start; Nit: " *" rather than "* " http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:37: while (VirtualQuery((LPCVOID)start, &mem, sizeof(mem))) { I'd suggest checking that the return value is either 0 or sizeof(mem), just in case. i.e. CHECK(result == sizeof(mem)) after. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:39: VirtualFree(start, 0, MEM_RELEASE); Please check return value. You can use LOG_FATAL on error. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:44: if ((LPVOID)start > sys_info.lpMaximumApplicationAddress) Minor nit: maybe '>='?
http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:14: const size_t kOneGb = 0x40000000; On 2011/08/16 01:21:49, Brad Chen wrote: > I'm used to 'b' meaning 'bit'. I'd rather see kOneGB here. Some folks find (1 << > 30) more readable than 0x40000000; your call. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:29: #elif NACL_WINDOWS && NACL_ARCH_CPU_32_BITS On 2011/08/16 17:34:10, Mark Seaborn wrote: > Can you use "NACL_BUILD_SUBARCH == 64" rather than "NACL_ARCH_CPU_32_BITS", for > consistency? No-one is using the latter. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:35: if (0 == NaCl_find_sandbox_memory(mem, kOneGb)) On 2011/08/16 17:34:10, Mark Seaborn wrote: > Nit: I'd use curly brackets for this 'if'. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:36: result = NaCl_page_alloc_at_addr(mem, addrsp_size); On 2011/08/16 17:28:38, Brad Chen wrote: > Nit: A brief comment before the call to NaCl_page_alloc_at_addr might be > helpful. > On 2011/08/16 17:21:02, bbudge1 wrote: > > On 2011/08/16 01:21:49, Brad Chen wrote: > > > I don't understand how mem gets initialized in this case. > > > > It's obscure, but the 'mem' parameter gets filled in by the call to > > NaCl_find_sandbox_memory if the pre-reserved region is found. Then the call to > > NaCl_page_alloc_at_addr tries to use that address. > Done. (I expanded the comment for the whole case) http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/sel_memory.h (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/sel_memory.h:30: int NaCl_find_sandbox_memory(void **p, On 2011/08/16 17:34:10, Mark Seaborn wrote: > Maybe name it something like "NaCl_find_prereserved_sandbox_memory()", to make > it a little clearer? Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... File src/trusted/service_runtime/win/sel_memory.c (right): http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:27: #if NACL_ARCH_CPU_32_BITS On 2011/08/16 17:34:10, Mark Seaborn wrote: > As before. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:29: int NaCl_find_sandbox_memory(void **p, On 2011/08/16 17:34:10, Mark Seaborn wrote: > Can you put a comment in to say that the purpose of this is to look for a region > reserved by the parent process on our behalf? Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:33: char* start; On 2011/08/16 17:34:10, Mark Seaborn wrote: > Nit: " *" rather than "* " Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:37: while (VirtualQuery((LPCVOID)start, &mem, sizeof(mem))) { On 2011/08/16 17:34:10, Mark Seaborn wrote: > I'd suggest checking that the return value is either 0 or sizeof(mem), just in > case. i.e. CHECK(result == sizeof(mem)) after. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:39: VirtualFree(start, 0, MEM_RELEASE); On 2011/08/16 17:34:10, Mark Seaborn wrote: > Please check return value. You can use LOG_FATAL on error. Done. http://codereview.chromium.org/7648002/diff/7001/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:44: if ((LPVOID)start > sys_info.lpMaximumApplicationAddress) On 2011/08/16 17:34:10, Mark Seaborn wrote: > Minor nit: maybe '>='? Done.
lgtm
LGTM with some nits. http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... File src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c (right): http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... src/trusted/service_runtime/arch/x86_32/sel_addrspace_x86_32.c:30: * On 32 bit Windows XP, a 1Gb region of address space is reserved before Just say "32-bit Windows". It's not just XP. http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... File src/trusted/service_runtime/win/sel_memory.c (right): http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:45: while (mem_size = VirtualQuery((LPCVOID)start, &mem, sizeof(mem))) { I wouldn't put assignments inside while() or if(). I wouldn't be surprised if compilers warn about this. I'd prefer: while (1) { size_t mem_size = VirtualQuery(...); if (mem_size == 0) { break; } CHECK(mem_size == sizeof(mem)); ... } http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:54: "NaCl_find_prereserved_sandbox_memory: VirtualFree(0x%016" Indent args to align with '('
http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... File src/trusted/service_runtime/win/sel_memory.c (right): http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:45: while (mem_size = VirtualQuery((LPCVOID)start, &mem, sizeof(mem))) { On 2011/08/16 20:18:43, Mark Seaborn wrote: > I wouldn't put assignments inside while() or if(). I wouldn't be surprised if > compilers warn about this. > > I'd prefer: > while (1) { > size_t mem_size = VirtualQuery(...); > if (mem_size == 0) { > break; > } > CHECK(mem_size == sizeof(mem)); > ... > } Done. http://codereview.chromium.org/7648002/diff/7003/src/trusted/service_runtime/... src/trusted/service_runtime/win/sel_memory.c:54: "NaCl_find_prereserved_sandbox_memory: VirtualFree(0x%016" On 2011/08/16 20:18:43, Mark Seaborn wrote: > Indent args to align with '(' Done.
Change committed as 6451 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
