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