> On Sept. 18, 2016, 10:42 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/launch.cpp, line 250 > > <https://reviews.apache.org/r/52011/diff/1/?file=1501198#file1501198line250> > > > > fork is linux specific. Please guard all pre_exec_commands with ifdef > > linux. > > Jie Yu wrote: > If we want to keep it on windows as well. I'd suggest we create a > `os::spawn` which is similar to os::system, but support argv style. This can > be implemented on both windows and linux (on windows, take a look at execvp > impl, `_spawnvp` is what you want), on linux, you can do the same as > os::system, but uses argv version if needed. > > Jie Yu wrote: > THis is in fact similar to posix_spawn > (http://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn.html)
I know benh was opposed to creating a version of `os:system()` that took an argv, because this goes against what the libc `system` call does (which just runs the passed in string in a shell). `posix_spawn()` looks more like what we want. Would you prefer me to do the full blown `spawn()` implementation or just wrap this in an `#ifdef linux` for now? - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52011/#review149397 ----------------------------------------------------------- On Sept. 18, 2016, 6:14 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52011/ > ----------------------------------------------------------- > > (Updated Sept. 18, 2016, 6:14 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-6075 > https://issues.apache.org/jira/browse/MESOS-6075 > > > Repository: mesos > > > Description > ------- > > Previously, we used 'process::subprocess()' to run all of our pre-exec > commands. However, doing so causes us to (unnecesssarily) initialize > all of libprocess (and subsequently creating a whole bunch of unused > threads, etc.) just to run a simple script. > > To avoid this, we now use fork/exec exec directly avoid calling > 'process:subprocess()'. In the past, we used 'os::system()' here > to avoid initializing libprocess, but this caused security issues with > allowing arbitrary shell commands to be appended to root-level > pre-exec commands that take strings as their last argument (e.g. mount > --bind <src> <target>, where target is user supplied and is set to > "target_dir; rm -rf /"). We now handle this case by invoking `execvp` > directly (which ensures that exactly one command is invoked and that > all of the strings passed to it are treated as direct arguments to > that one command). > > In the fututre, we should consider refactoring > 'libprocess::subprocess()' to pull the base functionality into stout > to avoid initializing libprocess for simple use cases like this one. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.cpp > fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 > > Diff: https://reviews.apache.org/r/52011/diff/ > > > Testing > ------- > > $ GTEST_FILTER="" make -j check > $ src/mesos-tests > $ sudo src/mesos-tests > > > Thanks, > > Kevin Klues > >
