> On Oct. 23, 2014, 11:03 a.m., Timothy Chen wrote: > > src/slave/slave.cpp, line 2567 > > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2567> > > > > Thanks for the fix! I wonder if we should just fix the command executor > > instead? > > > > I usually don't like mingling with user API input as it always confuses > > whoever using this, and leads to people start asking mailing lists and > > reading code because their arguemnts are changed. > > > > Shell=false was added for a reason and I think we shouldn't just change > > it for them, did you see why we can't just support this variation in the > > command executor?
>From a security perspective, shell=true is horrible as it requires the >framework user to correctly shell-escape arguments correctly. I Would hope that the mesos project would push frameworks to deprecate shell=true uses of command executor. That being said, i don't 100% follow your question, but I'll try to reexplain my motivation here with this bug. Running a command task on a slave requires the execution of an executor AND the execution of the Task. This is fine. What is busted here is that someone kludged the Executor launch to work by partly copying launch information from the normal Task launch configuration probably because it made sense at the time. This was fine when the task was totally defined by the single string "value" field ultimately passed to "sh -c". The executor launch simply replaced the "value" and then mesos-executor runs just fine with no arguments. But when the multivalued task launch (shell=false) was introduced, this launch hack breaks because the Task arguments are incorrectly given to mesos-executor when it launches. This really only breaks if you try to pass an arg with a double-dash, which fast-fails mesos-executor due to how it parses flags. If you want to revisit how the command executor works in general that's great, but i still think that the current functionality should be repaired first (the purpose of this patch). - R.B. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26622/#review57997 ----------------------------------------------------------- On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26622/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 11:40 p.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen. > > > Bugs: MESOS-1873 > https://issues.apache.org/jira/browse/MESOS-1873 > > > Repository: mesos-git > > > Description > ------- > > Basically if you use "shell=false" with a non-empty argument list and the > Command Executor it is completely broken. > > When we clone the env vars to fork "mesos-executor" all of the original cmd > args are cloned as well (unintentionally) due to some protocol-buffer merge > shenanigans. > > Don't pass task-related arguments to mesos-executor. > > The description on the linked jira ticket goes into more detail. > > > Diffs > ----- > > src/slave/slave.cpp 0e342ed > src/tests/slave_tests.cpp f585bdd > > Diff: https://reviews.apache.org/r/26622/diff/ > > > Testing > ------- > > make check > > added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired > behavior. > > > Thanks, > > R.B. Boyer > >
