> 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)
> 
> Kevin Klues wrote:
>     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?

spawn should be pretty straight forward to implement for both linux and 
windows. I'd prefer just do it to avoid doing a big change to wrap 
pre_exec_commands with ifdef linux.


- Jie


-----------------------------------------------------------
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
> 
>

Reply via email to