Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(61)

Issue 7795010: Use chain-loading for Linux nacl_helper (Closed)

Created:
9 years, 3 months ago by Roland McGrath
Modified:
9 years, 3 months ago
CC:
chromium-reviews, native-client-reviews_googlegroups.com
Visibility:
Public.

Description

Use chain-loading for Linux nacl_helper This replaces the nacl_helper_bootstrap program, dynamically-linked against nacl_helper.so, with a standalone, statically-linked nacl_helper_bootstrap program that loads the dynamic linker, instructing it in turn to load the nacl_helper program (now a PIE rather than a DSO). This avoids two problems with the old scheme: 1. The nacl_helper_bootstrap program remained in the dynamic linker's list of loaded objects, as the main executable, even though the memory where its .dynamic section had been was overwritten with the NaCl untrusted address space. Code that traverses the list of all loaded objects could thus attempt to look at pointers into this part of memory, and be led astray. 2. nacl_helper_bootstrap's large (~1G) bss segment could cause the kernel to refuse to load the program because it didn't think there was enough free memory in the system for so large an allocation of anonymous memory. The bootstrap program is kept very small by avoiding all use of libc (except for memset and integer division routines needed on ARM). It has its own custom start-up code hand-written in assembly and its own custom system call stubs done with hand-written GCC inline asm statements. To avoid the second problem, the bootstrap program no longer has a large bss. Instead, it has a special ELF segment (i.e. PT_LOAD header) that specifies no memory access, and a large (~1G) mapping size from the file. This mapping is way off the end of the file, but the kernel doesn't mind that, and since it's all a file mapping, the kernel does not do its normal memory accounting for consuming a large amount of anonymous memory. Unfortunately, it's impossible to get the linker to produce exactly the right PT_LOAD header by itself. Using a custom linker script, we get the layout exactly how we want it and a PT_LOAD header that is almost right. We then use a build-time helper program to munge one field of the PT_LOAD to make it exactly what we need. BUG= http://code.google.com/p/chromium/issues/detail?id=94147 TEST= hand-tested chromium build, invoked with --nacl-linux-helper R=bradchen@google.com,mseaborn@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98909

Patch Set 1 #

Total comments: 14

Patch Set 2 : trivia + use linux_syscall_support.h #

Patch Set 3 : more review nits #

Patch Set 4 : rebased #

Patch Set 5 : fix mmap use for x86-64 #

Total comments: 32

Patch Set 6 : review nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+734 lines, -76 lines) Patch
M build/install-build-deps.sh View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/chrome_paths.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_paths.cc View 2 chunks +8 lines, -1 line 0 comments Download
M chrome/nacl.gypi View 1 2 3 4 5 3 chunks +93 lines, -23 lines 0 comments Download
M chrome/nacl/nacl_fork_delegate_linux.cc View 2 chunks +10 lines, -8 lines 0 comments Download
M chrome/nacl/nacl_helper_bootstrap_linux.c View 1 2 3 4 5 1 chunk +419 lines, -14 lines 0 comments Download
A chrome/nacl/nacl_helper_bootstrap_linux.x View 1 2 3 4 5 1 chunk +93 lines, -0 lines 0 comments Download
A chrome/nacl/nacl_helper_bootstrap_munge_phdr.c View 1 1 chunk +66 lines, -0 lines 0 comments Download
A chrome/nacl/nacl_helper_bootstrap_munge_phdr.py View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
D chrome/nacl/nacl_helper_exports.txt View 1 chunk +0 lines, -10 lines 0 comments Download
M chrome/nacl/nacl_helper_linux.cc View 2 chunks +7 lines, -19 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
Roland McGrath
9 years, 3 months ago (2011-08-29 22:07:56 UTC) #1
Mark Seaborn
Initial comments... I think the code for the ELF chainloader should live in the NaCl ...
9 years, 3 months ago (2011-08-29 22:26:50 UTC) #2
Brad Chen
While I'm reading send this out to the try bots. In addition to the defaults ...
9 years, 3 months ago (2011-08-29 22:26:57 UTC) #3
Brad Chen
While I'm reading send this out to the try bots. In addition to the defaults ...
9 years, 3 months ago (2011-08-29 22:27:00 UTC) #4
Brad Chen
http://codereview.chromium.org/7795010/diff/1/chrome/nacl/nacl_helper_bootstrap_linux.c File chrome/nacl/nacl_helper_bootstrap_linux.c (right): http://codereview.chromium.org/7795010/diff/1/chrome/nacl/nacl_helper_bootstrap_linux.c#newcode5 chrome/nacl/nacl_helper_bootstrap_linux.c:5: * This is a standalone program that loads and ...
9 years, 3 months ago (2011-08-29 23:04:57 UTC) #5
Brad Chen
one more thing... http://codereview.chromium.org/7795010/diff/1/chrome/nacl.gypi File chrome/nacl.gypi (right): http://codereview.chromium.org/7795010/diff/1/chrome/nacl.gypi#newcode186 chrome/nacl.gypi:186: 'target_name': 'nacl_helper', Once this is in ...
9 years, 3 months ago (2011-08-29 23:25:24 UTC) #6
Roland McGrath
On 2011/08/29 22:26:50, Mark Seaborn wrote: > I think the code for the ELF chainloader ...
9 years, 3 months ago (2011-08-29 23:49:44 UTC) #7
Roland McGrath
On 2011/08/29 23:04:57, Brad Chen wrote: > http://codereview.chromium.org/7795010/diff/1/chrome/nacl/nacl_helper_bootstrap_linux.c#newcode5 > chrome/nacl/nacl_helper_bootstrap_linux.c:5: * This is a standalone ...
9 years, 3 months ago (2011-08-29 23:59:17 UTC) #8
Brad Chen
LGTM Please give Mark a chance to chime in, in case there is anything herein ...
9 years, 3 months ago (2011-08-30 00:29:44 UTC) #9
Mark Seaborn
In the "TEST=" field, please say how you invoke Chromium, i.e. the env vars needed ...
9 years, 3 months ago (2011-08-30 19:58:59 UTC) #10
Mark Seaborn
A few more comments... """ This avoids two problems with the old scheme: 1. The ...
9 years, 3 months ago (2011-08-30 20:12:25 UTC) #11
Roland McGrath
I changed the nits that were reasonable, and ignored the comments I found excessive.
9 years, 3 months ago (2011-08-30 20:34:26 UTC) #12
Mark Seaborn
LGTM
9 years, 3 months ago (2011-08-30 20:48:58 UTC) #13
commit-bot: I haz the power
9 years, 3 months ago (2011-08-31 01:30:28 UTC) #14
Change committed as 98909

Powered by Google App Engine
This is Rietveld 408576698