Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
831363002:
Add the ability to run a callback between fork and exec. (Closed)
|
Created:
5 years, 11 months ago by rickyz (no longer on Chrome) Modified:
5 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the ability to run a callback between fork and exec.
This will be used along with user namespaces allow blocking the child
from execing until the uid and gid map has been written.
BUG=312380
Committed: https://crrev.com/a0b860bf256bc5c847eaa0533e01736c2844e771
Cr-Commit-Position: refs/heads/master@{#311925}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use a delegate class instead of a Closure. #
Total comments: 20
Patch Set 3 : More comments. #
Total comments: 2
Patch Set 4 : Improve pre_exec_delegate comment. #
Total comments: 4
Patch Set 5 : Remove resets. #Patch Set 6 : Readd closing read end. #
Messages
Total messages: 31 (8 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org
rickyz@chromium.org changed reviewers: + mdempsky@chromium.org
This provides a useful, powerful generic mechanism, but it would be easy to miss-use. The warning may be enough, but we could also consider not making the mechanism as generic and simply provide a parent-child synchronization primitive since it's what you need (for instance allow providing a file descriptor on which the child is guaranteed to wait). I would be happy either way, I think it would be useful to loop-in a base/ reviewer early to see what they think. mdempsky@: WDYT? https://chromiumcodereview.appspot.com/831363002/diff/1/base/process/launch.h File base/process/launch.h (right): https://chromiumcodereview.appspot.com/831363002/diff/1/base/process/launch.h... base/process/launch.h:131: Closure pre_exec_hook; If we go this route, we should make the warning even stronger. Code between fork() and exec() needs to be essentially async-signal safe. (And base::LaunchProcess() is already taking some worrisome liberties). It's also not obvious how to check that callbacks themselves will stay safe. Since, as per our discussion, you only need this to build a synchronization primitive (so that the child waits for the parent to signal a pipe), perhaps we should build that in this API instead of giving such a generic API. I'm not convinced either way, so let's see what mdempsky@ prefers and also get input from a base/ owner before changing everything. https://chromiumcodereview.appspot.com/831363002/diff/1/base/process/process_... File base/process/process_unittest.cc (right): https://chromiumcodereview.appspot.com/831363002/diff/1/base/process/process_... base/process/process_unittest.cc:236: int exit_code = kDummyExitCode; = 42?
On 2015/01/06 00:43:36, jln wrote: > mdempsky@: WDYT? I'm similarly apprehensive about enabling arbitrary code to run between fork+execve, but I could be convinced if we have enough one-off cases that it's the best solution. I'm more worried about whether base::Callback is async-signal safe, and whether we can rely on it staying that way in the future. Looking briefly at the API, it seems like there's a lot of support for binding smart pointers as arguments, which makes me worried about malloc/free issues. Would it be to constricting to use a plain C function pointer or maybe a delegate? I'd have a bit more confidence in either of those.
On 2015/01/06 01:02:35, mdempsky wrote: > On 2015/01/06 00:43:36, jln wrote: > > mdempsky@: WDYT? > > I'm similarly apprehensive about enabling arbitrary code to run between > fork+execve, but I could be convinced if we have enough one-off cases that it's > the best solution. > > I'm more worried about whether base::Callback is async-signal safe, and whether > we can rely on it staying that way in the future. Looking briefly at the API, > it seems like there's a lot of support for binding smart pointers as arguments, > which makes me worried about malloc/free issues. > > Would it be to constricting to use a plain C function pointer or maybe a > delegate? I'd have a bit more confidence in either of those. Hm, at the moment, I can't think of another use case (though it's hard to predict the future). On the other hand, specifically supporting this synchronization case ends up with a slightly ugly API, where the caller has to deal with pipe fds (or alternatively, we raise(SIGSTOP) and the caller would have to do waitpid(pid, &status, WSTOPPED)/kill(SIGCONT), with the downside that there's no guarantee the SIGCONT didn't come from some other random process). If we stay with the function pointer or equivalent, we'd probably want a delegate over a function pointer, since it'd be nicer for passing state to it. For what it's worth, since this is a callback with no arguments and callbacks can be called multiple times, we aren't likely to hit any non async-signal-safe stuff, but yeah, not sure how correct it is to depend on this.
On 2015/01/06 02:52:34, rickyz wrote: > If we stay with the function pointer or equivalent, we'd probably want a > delegate over a function pointer, since it'd be nicer for passing state to it. > > For what it's worth, since this is a callback with no arguments and callbacks > can be called multiple times, we aren't likely to hit any non async-signal-safe > stuff, but yeah, not sure how correct it is to depend on this. I agree that a delegate looks like a good compromise. It gives enough flexibility without too much sugar and I'm more confident that we can make it clear that inheriting a AsyncSafeRunnerInterface, or something along these lines, will means users will read the comments of the class.
rickyz@chromium.org changed reviewers: + thestig@chromium.org
rickyz@chromium.org changed reviewers: + mark@chromium.org
(whoever gets to it first)
jln@chromium.org changed reviewers: - mark@chromium.org, thestig@chromium.org
Usually it's better to wait until you get a reviewer to approve a CL before moving-on to OWNERS. I've removed thestig/mark for now so that they don't get spammed. I've sent a few nits. https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... File base/process/launch.h (right): https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... base/process/launch.h:46: virtual ~PreExecDelegate() {} Style: add trivial constructor https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... base/process/launch.h:51: }; Style: DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... base/process/launch.h:52: #endif style: // defined(OS_POSIX) https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... base/process/launch.h:142: // presence of multiple threads, this essentially needs to be async-signal s/this/"code running in this delegate" https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/laun... base/process/launch.h:144: PreExecDelegate* pre_exec_delegate; scoped_ptr? https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/proc... File base/process/process_unittest.cc (right): https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/proc... base/process/process_unittest.cc:221: }; Style: DISALLOW_COPY_AND_ASSIGN https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/proc... base/process/process_unittest.cc:227: const int read_fd = pipe_fds[0]; How about making these ScopedFDs instead? That way we don't leak if an assert fails. https://chromiumcodereview.appspot.com/831363002/diff/20001/base/process/proc... base/process/process_unittest.cc:247: #endif Style: // defined(OS_POSIX)
https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:144: PreExecDelegate* pre_exec_delegate; On 2015/01/07 00:40:37, jln wrote: > scoped_ptr? scoped_ptr would mean the LaunchOptions take ownership of PreExecDelegate and that it has to be destroyed/recreated for each launched process. I think it makes sense to use a non-owning raw pointer here (but that's a weak opinion). https://codereview.chromium.org/831363002/diff/20001/base/process/launch_posi... File base/process/launch_posix.cc (right): https://codereview.chromium.org/831363002/diff/20001/base/process/launch_posi... base/process/launch_posix.cc:23: #include "base/callback.h" Not needed anymore.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:46: virtual ~PreExecDelegate() {} On 2015/01/07 00:40:37, jln wrote: > Style: add trivial constructor Done. https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:51: }; On 2015/01/07 00:40:37, jln wrote: > Style: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:52: #endif On 2015/01/07 00:40:37, jln wrote: > style: // defined(OS_POSIX) Done. https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:142: // presence of multiple threads, this essentially needs to be async-signal On 2015/01/07 00:40:37, jln wrote: > s/this/"code running in this delegate" Done. https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:144: PreExecDelegate* pre_exec_delegate; On 2015/01/07 01:03:50, mdempsky wrote: > On 2015/01/07 00:40:37, jln wrote: > > scoped_ptr? > > scoped_ptr would mean the LaunchOptions take ownership of PreExecDelegate and > that it has to be destroyed/recreated for each launched process. I think it > makes sense to use a non-owning raw pointer here (but that's a weak opinion). scoped_ptr doesn't work here because we make copies of LaunchOptions in some places (https://code.google.com/p/chromium/codesearch#chromium/src/base/test/launcher... and https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc...). Actually, I'm not too happy with how this interface turned out, since the presence of pre_exec_delegate could make the LaunchOptions potentially non-reusable. Perhaps this functionality belongs in a new argument to LaunchProcess rather than in LaunchOptions, since the delegate is likely to contain state specific to a particular invocation of LaunchProcess. I guess it depends on how LaunchOptions is meant to be viewed - is it something that's always expected to be tied to a single call to LaunchProcess, or is it something that should be reusable? https://codereview.chromium.org/831363002/diff/20001/base/process/launch_posi... File base/process/launch_posix.cc (right): https://codereview.chromium.org/831363002/diff/20001/base/process/launch_posi... base/process/launch_posix.cc:23: #include "base/callback.h" On 2015/01/07 01:03:50, mdempsky wrote: > Not needed anymore. Done. https://codereview.chromium.org/831363002/diff/20001/base/process/process_uni... File base/process/process_unittest.cc (right): https://codereview.chromium.org/831363002/diff/20001/base/process/process_uni... base/process/process_unittest.cc:221: }; On 2015/01/07 00:40:37, jln wrote: > Style: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/831363002/diff/20001/base/process/process_uni... base/process/process_unittest.cc:227: const int read_fd = pipe_fds[0]; On 2015/01/07 00:40:38, jln wrote: > How about making these ScopedFDs instead? That way we don't leak if an assert > fails. Sure - I kept the raw fd in the delegate because ScopedFD uses PCHECK. https://codereview.chromium.org/831363002/diff/20001/base/process/process_uni... base/process/process_unittest.cc:247: #endif On 2015/01/07 00:40:38, jln wrote: > Style: // defined(OS_POSIX) Done.
https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/831363002/diff/20001/base/process/launch.h#ne... base/process/launch.h:144: PreExecDelegate* pre_exec_delegate; On 2015/01/07 01:39:07, rickyz wrote: > Actually, I'm not too happy with how this interface turned out, since the > presence of pre_exec_delegate could make the LaunchOptions potentially > non-reusable. Perhaps this functionality belongs in a new argument to > LaunchProcess rather than in LaunchOptions, since the delegate is likely to > contain state specific to a particular invocation of LaunchProcess. > > I guess it depends on how LaunchOptions is meant to be viewed - is it something > that's always expected to be tied to a single call to LaunchProcess, or is it > something that should be reusable? It seems intuitive to me that if you reuse the same delegate for multiple calls to LaunchProcess(), then that delegate needs to be safe to be called once per LaunchProcess(). That said I'm open to alternatives if you think something else is better.
FWIW, LaunchOptions instances are never reused in existing code, but there's nothing preventing someone from reusing them. https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h#ne... base/process/launch.h:147: // needs to be async-signal safe. Maybe mention "man 7 signal" for a list of async signal safe functions, and explicitly mention no mallocing. Have "Warning" in caps because there be dragons.
On 2015/01/08 23:07:07, Lei Zhang wrote: > FWIW, LaunchOptions instances are never reused in existing code, but there's > nothing preventing someone from reusing them. > > https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h > File base/process/launch.h (right): > > https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h#ne... > base/process/launch.h:147: // needs to be async-signal safe. > Maybe mention "man 7 signal" for a list of async signal safe functions, and > explicitly mention no mallocing. Have "Warning" in caps because there be > dragons. Hm, based on Matthew's comment, it seems like the expectation here would be that users of pre_exec_delegate should be aware of whether their delegate is reusable and decide whether or not to reuse the LaunchOptions appropriately. I guess I'm fine with that if others are as well.
Oops, forgot to publish comment, sorry. https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/831363002/diff/60001/base/process/launch.h#ne... base/process/launch.h:147: // needs to be async-signal safe. On 2015/01/08 23:07:07, Lei Zhang wrote: > Maybe mention "man 7 signal" for a list of async signal safe functions, and > explicitly mention no mallocing. Have "Warning" in caps because there be > dragons. Done.
lgtm https://chromiumcodereview.appspot.com/831363002/diff/80001/base/process/proc... File base/process/process_unittest.cc (right): https://chromiumcodereview.appspot.com/831363002/diff/80001/base/process/proc... base/process/process_unittest.cc:243: write_fd.reset(); I don't think that's needed, right?
rickyz@chromium.org changed reviewers: + thestig@chromium.org
Thanks, readding thestig@ for base/ OWNERS. https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... File base/process/process_unittest.cc (right): https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... base/process/process_unittest.cc:243: write_fd.reset(); On 2015/01/15 19:42:56, jln wrote: > I don't think that's needed, right? Yeah, neither of the resets are technically needed, I was just closing the ends as soon as I was done with them.
https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... File base/process/process_unittest.cc (right): https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... base/process/process_unittest.cc:243: write_fd.reset(); On 2015/01/15 22:03:03, rickyz wrote: > On 2015/01/15 19:42:56, jln wrote: > > I don't think that's needed, right? > Yeah, neither of the resets are technically needed, I was just closing the ends > as soon as I was done with them. The first one is needed so that you get SIGPIPE-ed if the child process dies for whatever reason rather than risk a blocking write. (In practice we of course know that you don't risk a blocking write given that you're writing much less that a pipe's capacity, but the pattern is correct). I would remove the second one, but your choice since it doesn't matter.
https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... File base/process/process_unittest.cc (right): https://codereview.chromium.org/831363002/diff/80001/base/process/process_uni... base/process/process_unittest.cc:243: write_fd.reset(); On 2015/01/15 22:06:48, jln wrote: > On 2015/01/15 22:03:03, rickyz wrote: > > On 2015/01/15 19:42:56, jln wrote: > > > I don't think that's needed, right? > > Yeah, neither of the resets are technically needed, I was just closing the > ends > > as soon as I was done with them. > > The first one is needed so that you get SIGPIPE-ed if the child process dies for > whatever reason rather than risk a blocking write. > (In practice we of course know that you don't risk a blocking write given that > you're writing much less that a pipe's capacity, but the pattern is correct). > > I would remove the second one, but your choice since it doesn't matter. Ahh, I actually had not realized that subtle detail, thanks, readded that one.
lgtm
The CQ bit was checked by rickyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/831363002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a0b860bf256bc5c847eaa0533e01736c2844e771 Cr-Commit-Position: refs/heads/master@{#311925} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
