[
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: [email protected]
For additional commands, e-mail: [email protected]
