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

Issue 853583002: Convert DropFileSystemAccess to use ForkWithFlags. (Closed)

Created:
5 years, 11 months ago by rickyz (no longer on Chrome)
Modified:
5 years, 11 months ago
CC:
chromium-reviews, jln+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert DropFileSystemAccess to use ForkWithFlags. Previously, this used a thread, but this did not work within a PID namespace because /proc/<tid> can refer to a completely different process. BUG=312380 Committed: https://crrev.com/cbe41b9dda56f3f34bdf9012c5564b5cd15058e2 Cr-Commit-Position: refs/heads/master@{#311771}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Use RAW_CHECK in the child. #

Total comments: 6

Patch Set 3 : Respond to comments. #

Total comments: 4

Patch Set 4 : More comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -48 lines) Patch
M sandbox/linux/services/credentials.cc View 1 2 3 3 chunks +29 lines, -40 lines 0 comments Download
M sandbox/linux/services/credentials_unittest.cc View 3 chunks +2 lines, -8 lines 0 comments Download

Messages

Total messages: 15 (2 generated)
rickyz (no longer on Chrome)
5 years, 11 months ago (2015-01-14 00:17:32 UTC) #2
jln (very slow on Chromium)
Some early comment. In the previous version, we only need CAP_SYS_CHROOT. In the new version ...
5 years, 11 months ago (2015-01-14 00:57:10 UTC) #3
rickyz (no longer on Chrome)
On 2015/01/14 00:57:10, jln wrote: > Some early comment. > > In the previous version, ...
5 years, 11 months ago (2015-01-15 01:00:15 UTC) #4
rickyz (no longer on Chrome)
https://codereview.chromium.org/853583002/diff/1/sandbox/linux/services/credentials.cc File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/853583002/diff/1/sandbox/linux/services/credentials.cc#newcode119 sandbox/linux/services/credentials.cc:119: PCHECK(unshare(CLONE_FILES) == 0); On 2015/01/14 00:57:10, jln wrote: > ...
5 years, 11 months ago (2015-01-15 01:00:25 UTC) #5
jln (very slow on Chromium)
https://chromiumcodereview.appspot.com/853583002/diff/20001/sandbox/linux/services/credentials.cc File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/853583002/diff/20001/sandbox/linux/services/credentials.cc#newcode115 sandbox/linux/services/credentials.cc:115: pid_t pid = base::ForkWithFlags(SIGCHLD | CLONE_FS, nullptr, nullptr); This ...
5 years, 11 months ago (2015-01-15 19:29:44 UTC) #6
rickyz (no longer on Chrome)
https://codereview.chromium.org/853583002/diff/20001/sandbox/linux/services/credentials.cc File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/853583002/diff/20001/sandbox/linux/services/credentials.cc#newcode115 sandbox/linux/services/credentials.cc:115: pid_t pid = base::ForkWithFlags(SIGCHLD | CLONE_FS, nullptr, nullptr); On ...
5 years, 11 months ago (2015-01-15 22:19:05 UTC) #7
jln (very slow on Chromium)
lgtm https://chromiumcodereview.appspot.com/853583002/diff/40001/sandbox/linux/services/credentials.cc File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/853583002/diff/40001/sandbox/linux/services/credentials.cc#newcode118 sandbox/linux/services/credentials.cc:118: RAW_CHECK(chroot("/proc/self/fdinfo") == 0); Nit: I would write /proc/self/fdinfo/ ...
5 years, 11 months ago (2015-01-15 22:32:48 UTC) #8
rickyz (no longer on Chrome)
https://codereview.chromium.org/853583002/diff/40001/sandbox/linux/services/credentials.cc File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/853583002/diff/40001/sandbox/linux/services/credentials.cc#newcode118 sandbox/linux/services/credentials.cc:118: RAW_CHECK(chroot("/proc/self/fdinfo") == 0); On 2015/01/15 22:32:48, jln wrote: > ...
5 years, 11 months ago (2015-01-15 22:45:08 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/853583002/60001
5 years, 11 months ago (2015-01-15 22:54:32 UTC) #11
jln (very slow on Chromium)
Let's keep this committing, but: 1. I think we could still use a thread (but ...
5 years, 11 months ago (2015-01-15 23:50:41 UTC) #12
chromium-reviews
Hm, are you sure readlink on /proc/self would give the TID vs. the PID? On ...
5 years, 11 months ago (2015-01-16 00:00:15 UTC) #13
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 11 months ago (2015-01-16 00:28:55 UTC) #14
commit-bot: I haz the power
5 years, 11 months ago (2015-01-16 00:30:49 UTC) #15
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/cbe41b9dda56f3f34bdf9012c5564b5cd15058e2
Cr-Commit-Position: refs/heads/master@{#311771}

Powered by Google App Engine
This is Rietveld 408576698