Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1248673004:
Set a new TLS when using CLONE_VM. (Closed)
|
Created:
5 years, 5 months ago by rickyz (no longer on Chrome) Modified:
5 years, 5 months ago Reviewers:
jln (very slow on Chromium) CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet a new TLS when using CLONE_VM.
On some libcs, clone writes to the new child's TLS before returning, so
we set a new TLS to avoid corrupting the parent process's TLS.
On glibc, this caused an assertion failure inside of
pthread_getattr_np(pthread_self(), &attr).
BUG=512623
Committed: https://crrev.com/d2516cf1c3843541c920a476315e2a6f10cb0f92
Cr-Commit-Position: refs/heads/master@{#340025}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 9
Patch Set 3 : Respond to comments #
Total comments: 2
Patch Set 4 : Respond to comments. #Messages
Total messages: 16 (4 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org
Here's the workaround for the CLONE_VFORK issue :-(
https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:56: int ChrootToSelfFdinfo(void*) { Maybe add a comment here saying that this function needs to be very low level as it's running from a vfork-ed child. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:105: // The stack grows down on these architectures. Place the TLS at the bottom of "bottom of the stack" is very confusing here. I would say "top of the stack" and maybe qualify with "(lowest address of the stack)". I wonder if we shouldn't simply have another buffer though. And initialize it to all-zero. I'm worried we could get inconsistent behavior if something called from ChrootToSelfFdinfo starts accessing the TCB / TLS. At the very least we should fail consistently. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials_unittest.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:252: Could you add another test that creates a thread that looks at getpid() and sys_gettid() and checks that they're ok?
https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:97: defined(ARCH_CPU_MIPS_FAMILY) Let's just exclude MIPS here. We've not tested there and there is no need to add this complexity on that architecture.
https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:56: int ChrootToSelfFdinfo(void*) { On 2015/07/21 23:39:17, jln wrote: > Maybe add a comment here saying that this function needs to be very low level as > it's running from a vfork-ed child. Done. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:97: defined(ARCH_CPU_MIPS_FAMILY) On 2015/07/22 00:04:20, jln wrote: > Let's just exclude MIPS here. We've not tested there and there is no need to add > this complexity on that architecture. Done - also noticed that I forgot to add ARM in this list. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:105: // The stack grows down on these architectures. Place the TLS at the bottom of On 2015/07/21 23:39:17, jln wrote: > "bottom of the stack" is very confusing here. I would say "top of the stack" and > maybe qualify with "(lowest address of the stack)". > > I wonder if we shouldn't simply have another buffer though. And initialize it to > all-zero. > > I'm worried we could get inconsistent behavior if something called from > ChrootToSelfFdinfo starts accessing the TCB / TLS. At the very least we should > fail consistently. Yeah, I ended up switching this to a zero-initialized buffer - definitely better than reading uninit memory. Unfortunately, there isn't much guidance given on minimum TLS size. PTHREAD_STACK_MIN should always be more than sufficient for glibc's TLS usage. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials_unittest.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:252: On 2015/07/21 23:39:17, jln wrote: > Could you add another test that creates a thread that looks at getpid() and > sys_gettid() and checks that they're ok? Sorry, I wasn't quite sure what kind of test you were thinking about. If I create another thread, it'll have its own TLS, and the cached tid should be correct there. Even if the cached pid is incorrectly set to -1, getpid() will make a getpid syscall and update the cached value, so we wouldn't be able to tell that anything was amiss.
lgtm, but if you can think of more tests to add, please do! If you think you could test manually on ARM, that could be useful as we don't have good coverage there given the kernel support requirements. https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... File sandbox/linux/services/credentials_unittest.cc (right): https://codereview.chromium.org/1248673004/diff/20001/sandbox/linux/services/... sandbox/linux/services/credentials_unittest.cc:252: On 2015/07/22 04:00:32, rickyz wrote: > On 2015/07/21 23:39:17, jln wrote: > > Could you add another test that creates a thread that looks at getpid() and > > sys_gettid() and checks that they're ok? > Sorry, I wasn't quite sure what kind of test you were thinking about. If I > create another thread, it'll have its own TLS, and the cached tid should be > correct there. Even if the cached pid is incorrectly set to -1, getpid() will > make a getpid syscall and update the cached value, so we wouldn't be able to > tell that anything was amiss. Yeah, sorry I meant another "thread" with the vfork() we're doing. But that would require quite a bit of work to expose that stuff to testing and we don't really want to run too much code in that "thread" anyways. Is there anything we could test with raise() ? I vaguely remember raise() using some of the cached values in the TCB/PCB. https://codereview.chromium.org/1248673004/diff/40001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/1248673004/diff/40001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:98: void *tls = nullptr; Format nit: void*
Patchset #4 (id:60001) has been deleted
On 2015/07/22 17:30:06, jln wrote: > Yeah, sorry I meant another "thread" with the vfork() we're doing. But that > would require quite a bit of work to expose that stuff to testing and we don't > really want to run too much code in that "thread" anyways. > > Is there anything we could test with raise() ? I vaguely remember raise() using > some of the cached values in the TCB/PCB. Wow, good memory! raise does use the cached TID - added a test that raise works in DropFileSystemAccessPreservesTLS.
https://codereview.chromium.org/1248673004/diff/40001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/1248673004/diff/40001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:98: void *tls = nullptr; On 2015/07/22 17:30:06, jln wrote: > Format nit: void* Done.
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org Link to the patchset: https://codereview.chromium.org/1248673004/#ps80001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248673004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248673004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d2516cf1c3843541c920a476315e2a6f10cb0f92 Cr-Commit-Position: refs/heads/master@{#340025}
Message was sent while issue was closed.
On 2015/07/23 01:56:01, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/d2516cf1c3843541c920a476315e2a6f10cb0f92 > Cr-Commit-Position: refs/heads/master@{#340025} Hi Ricky, This CL has broken ASan Linux (sandboxed) tests: basically every test from browser_tests, content_browsertests, interactive_ui_tests times out: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%... The problem is perfectly reproducible locally if you build with GYP_DEFINES=asan=1. I'll take a look, but will revert your CL for now.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1256533002/ by glider@chromium.org. The reason for reverting is: This CL has broken ASan Linux (sandboxed) tests: basically every test from browser_tests, content_browsertests, interactive_ui_tests times out: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%.... |
