> On April 30, 2016, 1:04 a.m., Michael Park wrote: > > 3rdparty/libprocess/src/subprocess_windows.cpp, lines 305-352 > > <https://reviews.apache.org/r/46608/diff/1/?file=1358649#file1358649line305> > > > > (1) According to the "Security Remarks" section of > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx, > > it's recommended that we set the `lpApplicationName` parameter, rather > > than embedding the module name inside the `lpCommandLine` parameter since > > we need to make sure that the modulen name in `lpCommandLine` needs to be > > wrapped in quotes in case of spaces in the name. We don't seem to be doing > > this correctly. > > > > (2) `argumentsEscaped` is `new`ed, but not `delete`d, and according to > > the documentation for `CreateProcess`, it does not call `delete` on > > `lpCommandLine`. It looks like we're leaking? > > > > (3) Are we required to escape the quotes like we're doing here for > > `argumentsEscaped`...? I don't see anything like this in the documentation. > > > > What I would prefer to do here I think is to take a `argv` by value, > > then do: > > > > ```cpp > > string program = argv[0]; > > argv.erase(argv.begin()); > > string args = strings::join(" ", argv); > > > > ... = CreateProcess( > > program.data(), > > args.data(), > > ...); > > ``` > > > > If we wanted to avoid using `lpApplicationName` for whatever reason, > > how about: > > > > ```cpp > > string program = "\"" + argv[0] + "\""; > > argv.erase(argv.begin()); > > string args = strings::join(" ", argv); > > string cmd = strings::join(" ", program, args); > > > > ... = CreateProcess( > > NULL, > > cmd.data(), > > ...); > > ```
To follow up, (2) is resolved. (3) will have semantics that are identical to `cmd.exe` for most cases we care about, so I think we do not need to escape quotes. For (1) I think this is best addressed post MVP. As I mention in the `TODO` mapping arguments to `subprocess` into a `CreateProcess` call is nontrivial, and will need to be thought about carefully. We should come back to it post-MVP. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46608/#review131235 ----------------------------------------------------------- On May 10, 2016, 8:10 a.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46608/ > ----------------------------------------------------------- > > (Updated May 10, 2016, 8:10 a.m.) > > > Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, > Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun. > > > Bugs: MESOS-3637 > https://issues.apache.org/jira/browse/MESOS-3637 > > > Repository: mesos > > > Description > ------- > > Libprocess: Implemented `subprocess_windows.cpp`. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION > 3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION > 3rdparty/libprocess/src/io.cpp 83e5f04f246b46880cfc34aa56441046b569b142 > 3rdparty/libprocess/src/subprocess.cpp > bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 > 3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46608/diff/ > > > Testing > ------- > > > Thanks, > > Alex Clemmer > >
