hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Yang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-8207) Docker container launch use popen have risk of shell expansion
Date Tue, 08 May 2018 00:40:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16466683#comment-16466683
] 

Eric Yang commented on YARN-8207:
---------------------------------

[~jlowe] 

{quote}Rather than make an expensive deep copy of the arguments, construct_docker_command
only needs to copy the args vector then set the number of arguments to zero. At that point
we'd be effectively transferring ownership of the already allocated arg strings to the caller
without requiring full copies.{quote}

Struct args is still evolving.  I think it would be safer to keep the data structure private
for opaque data structure and deep copy to caller.  This avoids to put responsibility on external
caller to free internal implementation of struct args.  In case if we want to have ability
to trim or truncate the string array base on allowed parameters.  We have a way to fix it
later.

{quote}add_param_to_command_if_allowed (and many other places) doesn't check for make_string
failure, and add_to_args will segfault when it tries to dereference the NULL argument. Does
it make sense to have add_to_args return failure if the caller tried to add a NULL argument?{quote}

At this time, add_to_args returns no opts to avoid having to check for null on make_string.
 I think the proposal of making the reverse change will add more null pointer check, which
makes the code harder to read again.  It will contradict the original intend of your reviews
to make code easier to read.

{quote}flatten adds 1 to the strlen length in the loop, but there is only a need for one NUL
terminator which is already accounted for in the total initial value.{quote}

The +1 is for space, not NULL terminator for rendering html page that looks like a command
line.  The last space is replaced with NULL terminator.

{quote}flatten is using stpcpy incorrectly as it ignores the return values from the function.
stpcpy returns a pointer to the terminating NUL of the resulting string which is exactly what
we need for appending, so each invocation of stpcpy should be like: to = stpcpy(to, ...){quote}

This is fixed in YARN-7654 patch.  It's hard to rebase n times, and stuff gets to the wrong
patch.  I will fix this.

{quote}This change doesn't look related to the execv changes? Also looks like a case that
could be simplified quite a bit with strndup and strdup.{quote}

There is one byte off memory corruption that pattern is not null terminated properly.  This
was detected by valgrind, and I decided to fix this because it causes segfault if I leave
it in the code.

I will fix the rest of issues that you found.  Thank you again for the review.

> Docker container launch use popen have risk of shell expansion
> --------------------------------------------------------------
>
>                 Key: YARN-8207
>                 URL: https://issues.apache.org/jira/browse/YARN-8207
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: yarn-native-services
>    Affects Versions: 3.0.0, 3.1.0, 3.0.1, 3.0.2
>            Reporter: Eric Yang
>            Assignee: Eric Yang
>            Priority: Blocker
>              Labels: Docker
>         Attachments: YARN-8207.001.patch, YARN-8207.002.patch, YARN-8207.003.patch, YARN-8207.004.patch,
YARN-8207.005.patch, YARN-8207.006.patch, YARN-8207.007.patch
>
>
> Container-executor code utilize a string buffer to construct docker run command, and
pass the string buffer to popen for execution.  Popen spawn a shell to run the command.  Some
arguments for docker run are still vulnerable to shell expansion.  The possible solution is
to convert from char * buffer to string array for execv to avoid shell expansion.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message