[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[ https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16466512#comment-16466512 ] Eric Yang edited comment on YARN-8207 at 5/7/18 9:47 PM: - [~jlowe] Hadoop 3.1.1 release date was proposed for May 7th. This is a blocking issue for YARN-7654. I think this JIRA is very close to completion, and I like to make sure that we can catch the release train. Are you comfortable with the latest iteration of this patch? was (Author: eyang): [~jlowe] Hadoop 3.1.1 release date was proposed for May 7th. This is a blocking issue for YARN-7654. I think this JIRA is very close to completion, and I like to make sure that we can catch the release train. Are you comfortable to the last iteration of this patch? > 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-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[
https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16463048#comment-16463048
]
Jason Lowe edited comment on YARN-8207 at 5/4/18 2:24 PM:
--
Thanks for updating the patch! It's looking much better. I didn't finish
getting through the patch, but here's what I have so far.
MAX_RETRIES is unused and should be removed.
chosen_container_log_dir and init_log_dir are not used and should be removed.
In doing so we'll need to go back to freeing container_log_dir directly.
Nit: It would improve readability to use a typedef for the struct so we don't
have to keep putting "struct" everywhere it's used, e.g.:
{code}
typedef struct args {
[...]
} args_t;
{code}
or
{code}
typedef struct args args_t;
{code}
Nit: "out" is an odd name for a field in the args struct for the array of
arguments. Maybe just "args"? Similarly maybe "index" should be something
like "num_args" since that's what it is representing in the structure.
run_docker frees the vector of arguments but not the arguments themselves.
The following comment was updated but the permissions still appear to be 0700
in practice (and should be all that is required)?
{noformat}
- // Copy script file with permissions 700
+ // Copy script file with permissions 751
{noformat}
If the {{fork}} fails in launch_docker_container_as_user it would be good to
print strerror(errno) to the error file so there's a clue as to the nature of
the error.
Is there a reason not to use stpcpy in flatten? It would simplify it quite a
bit and eliminate the pointer arithmetic.
util.c has only whitespace changes.
docker-util.c added limits.h, but I don't think it was necessary.
add_to_args should be checking >= DOCKER_ARGS_MAX otherwise it will allow one
more arg than the buffer can hold.
add_to_args silently ignores (and leaks) the cloned argument if args->out is
NULL. args->out should not be null in practice. If a null check is deemed
useful then it should be at the beginning before work is done and return an
error to indicate the arg was not added. In any case it should only increment
the arg count when an argument was added.
free_args should set the argument count back to zero in case someone wants to
reuse the structure to build another argument list.
free_args should null out each argument pointer after it frees it. The
{{args->out[i] = NULL}} statement should be inside the {{for}} loop or it just
nulls out the element after the last which isn't very useful.
This previous comment still applies. Even though we are adjusting the index
the arg is not freed and nulled so it will end up being sent as an argument if
the args buffer is subsequently used (unless another argument is appended and
smashes it).
bq. add_param_to_command_if_allowed is trying to reset the index on errors, but
it fails to re-NULL out the written index values (if there were any). Either we
should assume all errors are fatal and therefore the buffer doesn't need to be
reset or the reset logic needs to be fixed.
Why do many of the get docker command functions smash the args count to zero?
IMHO the caller should be responsible for initializing the args structure. At
best the get docker command functions should be calling free_args rather than
smashing the arg count, otherwise they risk leaking any arguments that were
filled in.
get_docker_volume_command cleans up the arg structure on error but many other
get_docker_\*_command functions simply return with the args partially
initialized on error. This should be consistent.
get_docker_load_command should unconditionally call {{free(docker)}} then check
the return code for error since both code paths always call {{free(docker)}}.
Similar comment for get_docker_rm_command, get_docker_stop_command,
get_docker_kill_command, get_docker_run_command,
get_docker_kill_command allocates a 40-byte buffer to signal_buffer then
immediately leaks it.
Why does add_mounts cast string literals to (char*)? It compiles for me with
ro_suffix remaining const char*. If for some reason they need to be char* to
call make_string then it would be simpler to cast it at the call point rather
than at each initialization.
Nit: The end of get_mount_source should be simplified to just {{return
strndup(mount, len);}}
reset_args should call free_args or old arguments will be leaked.
reset_args does not clear the memory that was allocated, so when we try to use
args->out as an array terminated by a NULL pointer when we call exec it may not
actually be properly terminated. It should call calloc instead of malloc.
reset_args needs to allocate DOCKER_ARG_MAX + 1 pointers in order to hold
DOCKER_ARG_MAX arguments and still leave room for the NULL pointer terminator.
make_string does not check for vsnprintf returning an error.
was (Author: jlowe):
Thanks for updating the patch!
[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[
https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16463266#comment-16463266
]
Eric Yang edited comment on YARN-8207 at 5/4/18 1:34 AM:
-
[~jlowe] Thank you for the review. A couple comments:
Args is array of strings. Null terminator is not required for array when we
have length of the array. Hence, checking length > DOCKER_ARGS_MAX is fine.
Malloc without + 1 for null terminator for char** is okay. If someone write a
for loop without using index (length) variable for loop, it could cause
problems. Having said that, I will change the code to:
{code}
typedef struct {
int index;
char *out[DOCKER_ARG_MAX];
} args;
{code}
This can be easier to figure out the length of the actual array for other
developers. Container-executor is one time execution per exec. Args is not
reused, hence, the leak is not happening in practice. Args is only reused in
test cases. I plan to change reset_args to release the pointed strings and
assign NULL to each pointer rather than freeing the pointers. free(args);
would do the actual release of the args structure.
With the above change, I will also change get_docker_*_command to leave args in
partial state, and let caller decide to reset_args if return value is not 0.
was (Author: eyang):
[~jlowe] Thank you for the review. A couple comments:
Args is array of strings. Null terminator is not required for array when we
have length of the array. Hence, checking length > DOCKER_ARGS_MAX is fine.
Malloc without + 1 for null terminator for char** is okay. If someone write a
for loop without using index (length) variable for loop, it could cause
problems. Having said that, I will change the code to:
{code}
struct args {
int length;
char *out[DOCKER_ARG_MAX];
};
{code}
This can be easier to figure out the length of the actual array for other
developers. Container-executor is one time execution per exec. Args is not
reused, hence, the leak is not happening in practice. Args is only reused in
test cases. I plan to change reset_args to release the pointed strings and
assign NULL to each pointer rather than freeing the pointers. free(args);
would do the actual release of the args structure.
With the above change, I will also change get_docker_*_command to leave args in
partial state, and let caller decide to reset_args if return value is not 0.
> 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: Major
> Labels: Docker
> Attachments: YARN-8207.001.patch, YARN-8207.002.patch,
> YARN-8207.003.patch, YARN-8207.004.patch, YARN-8207.005.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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[
https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16459992#comment-16459992
]
Eric Yang edited comment on YARN-8207 at 5/1/18 6:44 PM:
-
{quote}How would the docker run be aborted? The container executor will leave
because the docker inspect retries maxed out, but won't the child thread
executing the docker run continue to run? That's how I think it would leak. I
didn't see how the child thread would be killed, it just looked like the parent
thread would give up and exit.{quote}
Docker remove is still called even if the container fail to start. If
container was left running during the creation process, yarn deletion service
will remove it forcefully.
{quote}That's not how the code works since the loop executes at most twice.
Essentially what it's doing is trying to sprintf into a 100-byte buffer, and if
that isn't the right size (as indicated by vsnprintf returning >= the specified
buffer size) then the code will reallocate the buffer to the required size as
specified by vsnprintf. It needs to add 1 to account for the terminating NUL,
but I don't see how adding yet another byte to the buffer is going to
significantly speed up the code since it will not reduce the number of
iterations of the loop.{quote}
You are right, and you mentioned this before. I made an error in my review of
the code. There is no need to +1 on the realloc. Your implementation doesn't
compile for me. The code on vsnprintf man page has memory leak as well. I
will refine your version to fit.
was (Author: eyang):
{quote}How would the docker run be aborted? The container executor will leave
because the docker inspect retries maxed out, but won't the child thread
executing the docker run continue to run? That's how I think it would leak. I
didn't see how the child thread would be killed, it just looked like the parent
thread would give up and exit.{quote}
Docker remove is still called even if the container fail to start. If
container was left running during the creation process, yarn deletion service
will remove it forcefully.
{quote}That's not how the code works since the loop executes at most twice.
Essentially what it's doing is trying to sprintf into a 100-byte buffer, and if
that isn't the right size (as indicated by vsnprintf returning >= the specified
buffer size) then the code will reallocate the buffer to the required size as
specified by vsnprintf. It needs to add 1 to account for the terminating NUL,
but I don't see how adding yet another byte to the buffer is going to
significantly speed up the code since it will not reduce the number of
iterations of the loop.{quote}
You are right, and you mentioned this before. I made an error in my review of
the code. There is no need to +1 on the realloc. Your implementation doesn't
compile for me. The code on vsnprintf has memory leak as well. I will refine
your version to fit.
> 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
>Reporter: Eric Yang
>Assignee: Eric Yang
>Priority: Major
> Attachments: YARN-8207.001.patch, YARN-8207.002.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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[
https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16457139#comment-16457139
]
Eric Yang edited comment on YARN-8207 at 4/27/18 11:06 PM:
---
[~jlowe] Thank you for the review. Good suggestions on coding style issues. I
will fix the coding style issues.
{quote}
stderr.txt is fopen'd and never used before the fclose? Are we supposed to dup2
these file descriptors to 1 and 2 before the execv so any errors from docker
run appear in those output files?{quote}
When using launch_script.sh, there is stdout and stderr redirection inside
launch_script.sh which bind-mount to host log directory. This is the reason
that there is fopen and fclosed immediately until YARN-7654 logic are added.
{quote}The parent process that is responsible for obtaining the pid is not
waiting for the child to complete before running the inspect command. That's
why retries had to be added to get it to work when they were not needed before.
The parent should simply wait and check for error exit codes as it did before
when it was using popen. After that we can ditch the retries since they won't
be necessary.{quote}
Using launch_script.sh, container-executor runs "docker run" with detach
option. It assumes the exit code can be obtained quickly. This is the reason
there is no logic for retry "docker inspect". This assumption is some what
flawed. If the docker image is unavailable on the host, docker will show
download progress and some other information and errors. The progression are
not captured, which is difficult to debug. When docker inspect is probed,
there is no information of what failed.
Without launch_script.sh, container-executor runs "docker run" in the
foreground, and obtain pid when the first process is started. Inspect command
is checked asynchronously because docker run exit code is only reported when
the docker process is terminated. There is a balance between how long that we
should wait before we decide if the system is hang. We can make MAX_RETRIES
configurable in case people have a difference preference of wait time for
docker inspect.
{quote}Why does make_string calculate size = n + 2 instead of n + 1?{quote}
This change makes make_string function twice faster than sample code while
waste 1% or less space if recursion is required. It is probably a reasonable
trade off for modern day computers.
was (Author: eyang):
[~jlowe] Thank you for the review. Good suggestions on coding style issues. I
will fix the coding style issues.
{quote}
stderr.txt is fopen'd and never used before the fclose? Are we supposed to dup2
these file descriptors to 1 and 2 before the execv so any errors from docker
run appear in those output files?{quote}
When using launch_script.sh, there is stdout and stderr redirection inside
launch_script.sh which bind-mount to host log directory. This is the reason
that there is fopen and fclosed immediately until YARN-7654 logic are added.
{quote}The parent process that is responsible for obtaining the pid is not
waiting for the child to complete before running the inspect command. That's
why retries had to be added to get it to work when they were not needed before.
The parent should simply wait and check for error exit codes as it did before
when it was using popen. After that we can ditch the retries since they won't
be necessary.{quote}
Using launch_script.sh, container-executor runs "docker run" with detach
option. It assumes the exit code can be obtained quickly. This is the reason
there is no logic for retry "docker inspect". This assumption is some what
flawed. If the docker image is unavailable on the host, docker will show
download progress and some other information and errors. The progression are
not captured, which is difficult to debug. When docker inspect is probed,
there is no information of what failed.
Without launch_script.sh, container-executor runs "docker run" in the
foreground, and obtain pid when the first process is started. Inspect command
is checked asynchronously because docker run exit code is only reported when
the docker process is terminated. There is a balance between how long that we
should wait before we decide if the system is hang. We can make MAX_RETRIES
configurable in case people like to wait for longer or period of time before
deciding if the container should fail.
{quote}Why does make_string calculate size = n + 2 instead of n + 1?{quote}
This change makes make_string function twice faster than sample code while
waste 1% or less space if recursion is required. It is probably a reasonable
trade off for modern day computers.
> Docker container launch use popen have risk of shell expansion
> --
>
> Key: YARN-8207
> URL: https://issues.apache.o
[jira] [Comment Edited] (YARN-8207) Docker container launch use popen have risk of shell expansion
[
https://issues.apache.org/jira/browse/YARN-8207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16457139#comment-16457139
]
Eric Yang edited comment on YARN-8207 at 4/27/18 11:03 PM:
---
[~jlowe] Thank you for the review. Good suggestions on coding style issues. I
will fix the coding style issues.
{quote}
stderr.txt is fopen'd and never used before the fclose? Are we supposed to dup2
these file descriptors to 1 and 2 before the execv so any errors from docker
run appear in those output files?{quote}
When using launch_script.sh, there is stdout and stderr redirection inside
launch_script.sh which bind-mount to host log directory. This is the reason
that there is fopen and fclosed immediately until YARN-7654 logic are added.
{quote}The parent process that is responsible for obtaining the pid is not
waiting for the child to complete before running the inspect command. That's
why retries had to be added to get it to work when they were not needed before.
The parent should simply wait and check for error exit codes as it did before
when it was using popen. After that we can ditch the retries since they won't
be necessary.{quote}
Using launch_script.sh, container-executor runs "docker run" with detach
option. It assumes the exit code can be obtained quickly. This is the reason
there is no logic for retry "docker inspect". This assumption is some what
flawed. If the docker image is unavailable on the host, docker will show
download progress and some other information and errors. The progression are
not captured, which is difficult to debug. When docker inspect is probed,
there is no information of what failed.
Without launch_script.sh, container-executor runs "docker run" in the
foreground, and obtain pid when the first process is started. Inspect command
is checked asynchronously because docker run exit code is only reported when
the docker process is terminated. There is a balance between how long that we
should wait before we decide if the system is hang. We can make MAX_RETRIES
configurable in case people like to wait for longer or period of time before
deciding if the container should fail.
{quote}Why does make_string calculate size = n + 2 instead of n + 1?{quote}
This change makes make_string function twice faster than sample code while
waste 1% or less space if recursion is required. It is probably a reasonable
trade off for modern day computers.
was (Author: eyang):
[~jlowe] Thank you for the review. Good suggestions on coding style issues. I
will fix the coding style issues.
{quote}
stderr.txt is fopen'd and never used before the fclose? Are we supposed to dup2
these file descriptors to 1 and 2 before the execv so any errors from docker
run appear in those output files?{quote}
When using launch_script.sh, there is stdout and stderr redirection inside
launch_script.sh which bind-mount to host log directory. This is the reason
that there is fopen and fclosed immediately until YARN-7654 logic are added.
{quote}The parent process that is responsible for obtaining the pid is not
waiting for the child to complete before running the inspect command. That's
why retries had to be added to get it to work when they were not needed before.
The parent should simply wait and check for error exit codes as it did before
when it was using popen. After that we can ditch the retries since they won't
be necessary.{quote}
Using launch_script.sh, container-executor runs "docker run" with detach
option. It assumes the exit code can be obtained quickly. This is the reason
there is no logic for retry "docker inspect". This assumption is some what
flawed. If the docker image is unavailable on the host, docker will show
download progress and some other information and errors. The progression are
not captured, which is difficult to debug. When docker inspect is probed,
there is no information of what failed.
Without launch_script.sh, container-executor runs "docker run" in the
foreground, and obtain pid when the first process is started. Inspect command
is checked asynchronously because docker run exit code is only reported when
the docker process is terminated. There is a balance between how long that we
should wait before we decide if the system is hang. We can make MAX_RETRIES
configurable in case people like to wait for longer or period of time before
deciding if the container should fail.
{quote}Why does make_string calculate size = n + 2 instead of n + 1?{quote}
This change make make_string function twice faster than sample code while waste
1% or less space if recursion is required. It is probably a reasonable trade
off for modern day computers.
> Docker container launch use popen have risk of shell expansion
> --
>
> Key: YARN-8207
> URL
