[jira] [Commented] (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=16467958#comment-16467958 ] Hudson commented on YARN-8207: -- SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #14142 (See [https://builds.apache.org/job/Hadoop-trunk-Commit/14142/]) YARN-8207. Docker container launch use popen have risk of shell (jlowe: rev a2ea7564209dce896a5badee3949ee41e4bc00bf) * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.h * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.h * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.c * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.h * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c > 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 > Fix For: 3.2.0, 3.1.1 > > 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, YARN-8207.008.patch, > YARN-8207.009.patch, YARN-8207.010.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] [Commented] (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=16467950#comment-16467950 ] Eric Yang commented on YARN-8207: - [~jlowe] Thank you for the persistent reviews to make this better. :) > 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 > Fix For: 3.2.0, 3.1.1 > > 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, YARN-8207.008.patch, > YARN-8207.009.patch, YARN-8207.010.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] [Commented] (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=16467936#comment-16467936 ] Jason Lowe commented on YARN-8207: -- Thanks for updating the patch! +1 for patch v10. Thanks for all your hard work on the patches. Committing this. > 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, YARN-8207.008.patch, > YARN-8207.009.patch, YARN-8207.010.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] [Commented] (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=16467869#comment-16467869
]
genericqa commented on YARN-8207:
-
| (/) *{color:green}+1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
30s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 23m
36s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
55s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
34s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
34m 11s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
33s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
50s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
50s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
50s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
31s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
10m 17s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 20m
36s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
22s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 68m 19s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922504/YARN-8207.010.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 4dbe379372f0 4.4.0-89-generic #112-Ubuntu SMP Mon Jul 31
19:38:41 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / d72c1651 |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20644/testReport/ |
| Max. process+thread count | 407 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20644/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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, YARN-8207.008.patch,
> YARN-8207.009.patch, YARN-8207.010.patch
>
>
> Container-executor code utilize a
[jira] [Commented] (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=16467853#comment-16467853 ] Eric Yang commented on YARN-8207: - [~jlowe] Patch 10 is posted. > 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, YARN-8207.008.patch, > YARN-8207.009.patch, YARN-8207.010.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] [Commented] (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=16467837#comment-16467837
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
40s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 37m
59s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
56s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
41s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
50m 16s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
41s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
57s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
57s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
57s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
39s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
12m 9s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 18m 39s{color}
| {color:red} hadoop-yarn-server-nodemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
22s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 85m 1s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.nodemanager.containermanager.scheduler.TestContainerSchedulerQueuing
|
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922491/YARN-8207.009.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 670c932cfb1c 3.13.0-141-generic #190-Ubuntu SMP Fri Jan 19
12:52:38 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / d72c1651 |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| unit |
https://builds.apache.org/job/PreCommit-YARN-Build/20641/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20641/testReport/ |
| Max. process+thread count | 357 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20641/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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:
[jira] [Commented] (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=16467783#comment-16467783
]
Jason Lowe commented on YARN-8207:
--
{quote}One concern about the shallow copy, struct args buffer supposedly
disappeared after construct_docker_command. This was the reason that I used
deep copy to extract the data. Now, I am retaining the pointer reference to
strings internal to struct args buffer instead of deep copy. Wouldn't those
strings get overwritten at some point or they will be reserved until copy is
freed up?
{quote}
I don't see patch 10 as of my writing this, so this is based on patch 9's
version of extract_execv_args.
The shallow copy in extract_execv_args is OK because that makes a copy of the
relevant thing that is disappearing when construct_docker_command completes.
When that stack frame disappears the contents of the {{struct args}} becomes
invalid, but that is only the length field and the data field. The data field
is only an array of character pointers and not the memory those character
pointers reference. That's why we only need to make a copy of the data array –
the individual arguments referenced by the data array were each malloc'd (many
via strdup or make_string), so each argument is still valid despite the
construct_docker_command stack frame disappearing.
If the character pointer array itself were malloc'd rather than stored on the
stack then we wouldn't even need to make the shallow copy, but I agree it's not
essential to fix that here.
I'll take a look at patch 10 when that's posted.
> 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, YARN-8207.008.patch,
> YARN-8207.009.patch, YARN-8207.010.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] [Commented] (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=16467762#comment-16467762 ] Eric Yang commented on YARN-8207: - [~jlowe] I see what you mean now, and patch 10 updated accordingly for initialize args and make_string check. One concern about the shallow copy, struct args buffer supposedly disappeared after construct_docker_command. This was the reason that I used deep copy to extract the data. Now, I am retaining the pointer reference to strings internal to struct args buffer instead of deep copy. Wouldn't those strings get overwritten at some point or they will be reserved until copy is freed up? > 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, YARN-8207.008.patch, > YARN-8207.009.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] [Commented] (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=16467750#comment-16467750 ] Jason Lowe commented on YARN-8207: -- Additional comment on patch 9: extract_execv_args needs to set args->length to zero to prevent the individual arguments from being freed if the caller subsequently calls reset_args to reuse the structure. > 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, YARN-8207.008.patch, > YARN-8207.009.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] [Commented] (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=16467727#comment-16467727 ] Eric Yang commented on YARN-8207: - [~jlowe] Patch 9 fixes most nits from your comments except init_args. I did not write init_args to prevent myself from making a mess. If you have strong feeling about the initialization. Please open a separate issue for it. Thanks > 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, YARN-8207.008.patch, > YARN-8207.009.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] [Commented] (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=16467725#comment-16467725
]
Jason Lowe commented on YARN-8207:
--
init_args would not require the structure to be malloc'd, rather it would look
like this when used:
{code:java}
args args;
init_args();
{code}
or it could be done like a macro, e.g.:
{code:java}
#define ARGS_INITIAL_VALUE { 0 }
[...]
args args = ARGS_INITIAL_VALUE;
{code}
The problem with it now is the reader (and caller) has to be intimately aware
of the layout of the args struct to understand what that line of code is doing.
Given that line appears multiple times in the patch, it should be fixed to
improve readability, encapsulation, etc.
{quote}No, it doesn't malloc(-1) will return null instead of 0 bytes, the
second check will not succeed.
{quote}
It won't be a malloc(-1), it will be a malloc(0) because it adds 1 to the
result of vsnprintf to calculate the buffer length. malloc(0) is not clearly
defined, but some systems will return a pointer that isn't NULL that can be
safely free()'d.
> 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, YARN-8207.008.patch,
> YARN-8207.009.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] [Commented] (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=16467673#comment-16467673
]
Eric Yang commented on YARN-8207:
-
[~jlowe]
{quote}At a bare minimum there should be a utility method, e.g:
extract_execv_args(args* args){quote}
I agree to this point, and will do this.
{quote}Please create an init function, e.g.: init_args(args* args), or a macro
to encapsulate initialization of the structure.{quote}
Init_args is only assigning 0 to length. I prefer to write it as:
{code}
struct args buffer = { 0 };
{code}
Instead of:
{code}
struct args *buffer = malloc(sizeof(args));
init_args(buffer);
{code}
I understand the desire and obsession for code perfection, but I am trying to
restrain myself from making more mess in crunch time.
{quote}As add_to_args works today, the lack of a NULL check on the make_string
result will cause the program to crash. {quote}
Sorry, I thought I had a null check, but it was changed to length check. This
will be fixed.
{quote}would be safer and easier to understand written with strdup/strndup,
e.g.:
{code}
dst = strndup(values[i], tmp_ptr - values[i]);
pattern = strdup(permitted_values[j] + 6);
{code}
{quote}
This will be optimized.
{quote}make_string is still not checking for vsnprintf failure. If the first
vsnprintf fails and returns -1, the code will allocate a 0-byte buffer.{quote}
No, it doesn't malloc(-1) will return null instead of 0 bytes, the second check
will not succeed.
> 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, YARN-8207.008.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] [Commented] (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=16467488#comment-16467488
]
Jason Lowe commented on YARN-8207:
--
{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.
{quote}
Having an opaque data structure that the caller must deep copy to use correctly
doesn't make sense. Deep copies by clients is the antithesis of opacity, since
the client must intimately understand the structure to properly perform the
deep copy. Even if the structure isn't changing, clients are likely to get this
copy wrong (as has already occurred), especially if each use needs it done
separately. At a bare minimum there should be a utility method, e.g:
{{extract_execv_args(args* args)}}, that encapsulates the extraction of execv
arguments so each use of args for execv doesn't require a separate, tricky deep
copy loop with a trailing NULL appended. That utility method could also avoid
the full deep copy and simply transfer ownership of the individual strings to
the shallow-copied vector that is returned. But in the end I think avoiding
copies completely by heap allocating the args is a much more efficient and
elegant solution.
Please create an init function, e.g.: init_args(args* args), or a macro to
encapsulate initialization of the structure. If the structure is likely to
change then it's important to put this in one place rather than chase down
every separate initialization code instance.
In {{flatten}} this code:
{code:java}
to = '\0';
{code}
is setting the pointer to NULL rather than terminating the string with a NUL
character. It needs to be:
{code:java}
*to = '\0';
{code}
{quote}At this time, add_to_args returns no opts to avoid having to check for
null on make_string.
{quote}
I'm not sure what is meant by "returns no opts to avoid having to check for
null on make_string." If add_to_args is passed a NULL string then it will
segfault because the first statement in add_to_args tries to dereference it:
{code:java}
static int add_to_args(args *args, const char *string) {
if (!string[0]) {
{code}
{quote}I think the proposal of making the reverse change will add more null
pointer check, which makes the code harder to read again.
{quote}
What I'm proposing would make the code easier to read without triggering the
segfault. Most uses of add_to_args look something like this:
{code:java}
char* x = make_string(fmt, ...);
ret = add_to_args(args, x);
free(x);
if (ret != 0) { ...
{code}
As add_to_args works today, the lack of a NULL check on the make_string result
will cause the program to crash. I'm proposing to add this check at the
beginning of add_to_args:
{code:java}
static int add_to_args(args *args, const char *string) {
if (string == NULL) {
return -1;
}
{code}
Then the existing make_string followed by add_to_args code pattern works
because add_to_args will properly handle a NULL argument and return an error.
That precludes the need to add NULL checks everywhere make_string is called
when immediately followed by add_to_args. That improves readability while
avoiding a segfault.
The length != 0 check in reset_args is unnecessary.
This code:
{code:java}
size_t offset = tmp_ptr - values[i] + 1;
dst = (char *) alloc_and_clear_memory(offset, sizeof(char));
strncpy(dst, values[i], offset);
dst[tmp_ptr - values[i]] = '\0';
pattern = (char *)
alloc_and_clear_memory((size_t)(strlen(permitted_values[j]) - 5), sizeof(char));
strcpy(pattern, permitted_values[j] + 6);
{code}
would be safer and easier to understand written with strdup/strndup, e.g.:
{code:java}
dst = strndup(values[i], tmp_ptr - values[i]);
pattern = strdup(permitted_values[j] + 6);
{code}
make_string is still not checking for vsnprintf failure. If the first vsnprintf
fails and returns -1, the code will allocate a 0-byte buffer. The second
vsnprintf may then actually succeed, returning the length required to store all
of the string without updating the zero-length buffer. Then make_string will
"succeed" by returning an unterminated buffer.
It would be good to fix the tab character issue that's been flagged in the past
few patches (in create_container_directories).
> 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:
[jira] [Commented] (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=16466771#comment-16466771
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
38s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 26m
57s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
55s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
37s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
38m 34s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
51s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
51s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
51s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
32s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} whitespace {color} | {color:red} 0m
0s{color} | {color:red} The patch 1 line(s) with tabs. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
11m 39s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 19m
31s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
25s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 73m 20s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922372/YARN-8207.008.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 8e11315a16fd 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9
14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 696a4be |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| whitespace |
https://builds.apache.org/job/PreCommit-YARN-Build/20624/artifact/out/whitespace-tabs.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20624/testReport/ |
| Max. process+thread count | 327 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20624/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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,
[jira] [Commented] (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=16466716#comment-16466716 ] Eric Yang commented on YARN-8207: - [~jlowe] Patch 008 fixed the issues discovered except char array copy. There is approximately 900kb leaks in container-executor prior to this patch, and we saved 20kb from leaking base on valgrind report exercising test cases. Execvp will wipe out all the leaks anyhow. Unless we find more of the buffer overflow problems. I am going to stop styling code changes because styling change has diminished return of investment at this point. > 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, YARN-8207.008.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] [Commented] (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=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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (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=16466649#comment-16466649
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
48s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 32m
39s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
55s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
39s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
44m 12s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
49s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
49s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
49s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
32s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} whitespace {color} | {color:red} 0m
0s{color} | {color:red} The patch 1 line(s) with tabs. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
11m 27s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 23m 28s{color}
| {color:red} hadoop-yarn-server-nodemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
27s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 83m 6s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.nodemanager.containermanager.scheduler.TestContainerSchedulerBehaviorCompatibility
|
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922274/YARN-8207.007.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 11ca01f6e274 3.13.0-137-generic #186-Ubuntu SMP Mon Dec 4
19:09:19 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 696a4be |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| whitespace |
https://builds.apache.org/job/PreCommit-YARN-Build/20622/artifact/out/whitespace-tabs.txt
|
| unit |
https://builds.apache.org/job/PreCommit-YARN-Build/20622/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20622/testReport/ |
| Max. process+thread count | 303 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20622/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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,
[jira] [Commented] (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=16466578#comment-16466578
]
Jason Lowe commented on YARN-8207:
--
Thanks for updating the patch!
Nit: Needing to initialize the args struct manually with '\{ 0 \}' each time is
fragile to maintain (e.g.: it could easily break as soon as someone adds a new
field). It would be good to add an init_args function to encapsulate the
initialization code or at least hide the init value in a macro.
There's an off-by-one error and heap corruption when construct_docker_command
places the terminating NULL. It allocates an array with buffer.length+1
elements then proceeds to write to index buffer.length+1 which is after the end
of the allocated array.
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.
Speaking of copying, given the args struct is always intended to be passed to
some flavor of execv, I think it would be easier to wield if the args struct
heap-allocated its argument vector and tracked the terminating NULL directly.
That way users don't have to perform a cumbersome and expensive copy of the
arguments or remember to terminate it before calling execv. It would also
preclude the need for free_char_arr since the existing free_values could be
used instead. I'm not sure the array in the structure is buying us much.
Speaking of free_char_arr, most uses should use free_values since the calls are
immediately followed by freeing the array pointer which free_values does.
launch_container_as_user does not free docker_command.
Nit: launch_container_as_user calls flatten but it's only necessary in the case
where the fork fails.
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.
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, ...)}}
flatten terminates the string by writing to buffer[total] but that is past the
end of the allocated array since it is only {{total} bytes in size. It should
be simply
{code}
*to = '\0';
{code}
reset_args and free_args are a NULL-check away from being the same thing, and
arguably free_args should check for NULL if reset_args does. That indicates we
only need one of these.
Nit: The args length != 0 check in free_args is unnecessary as the {{for}} loop
will essentially do the same.
check_trusted_image should be calling free_values instead of free_char_arr()
and free() on the get_configuration_values_delimiter result.
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?
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.
{noformat}
@@ -195,11 +218,11 @@ static int add_param_to_command_if_allowed(const struct
configuration *command_c
} else {
// If permitted-Values[j] is a REGEX, use REGEX to compare
if (is_regex(permitted_values[j])) {
- size_t offset = tmp_ptr - values[i];
+ size_t offset = tmp_ptr - values[i] + 1;
dst = (char *) alloc_and_clear_memory(offset, sizeof(char));
strncpy(dst, values[i], offset);
dst[tmp_ptr - values[i]] = '\0';
- pattern = (char *)
alloc_and_clear_memory((size_t)(strlen(permitted_values[j]) - 6), sizeof(char));
+ pattern = (char *)
alloc_and_clear_memory((size_t)(strlen(permitted_values[j]) - 5), sizeof(char));
strcpy(pattern, permitted_values[j] + 6);
ret = execute_regex_match(pattern, dst);
} else {
{noformat}
set_pid_namespace and set_privileged allocate an unused 1024-byte array.
Nit: The last {{goto free_and_exit}} in get_docker_kill_command is unnecessary.
In get_docker_start_command:
{code}
ret = add_to_args(args, container_name);
if (ret != 0) {
goto free_and_exit;
}
free_and_exit:
{code}
should be simplified to:
{code}
ret = add_to_args(args, container_name);
free_and_exit:
{code}
set_group_add should be calling free_values instad of free_char_arr or the
pointer array is leaked.
get_docker_stop_command does not check for add_to_args failure after trying to
add the
[jira] [Commented] (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=16466512#comment-16466512 ] Eric Yang commented on YARN-8207: - [~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] [Commented] (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=16464894#comment-16464894
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
24s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 22m
49s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
50s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
35s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
33m 29s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
31s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
47s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
47s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
47s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
31s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} whitespace {color} | {color:red} 0m
0s{color} | {color:red} The patch has 1 line(s) that end in whitespace. Use git
apply --whitespace=fix <>. Refer https://git-scm.com/docs/git-apply
{color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
10m 31s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 19m
31s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
23s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 66m 42s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922153/YARN-8207.006.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 3f90e50ee75c 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20
11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / a732acd |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| whitespace |
https://builds.apache.org/job/PreCommit-YARN-Build/20607/artifact/out/whitespace-eol.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20607/testReport/ |
| Max. process+thread count | 396 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20607/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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,
[jira] [Commented] (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=16464891#comment-16464891
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
32s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 26m
25s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
55s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
37s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
38m 12s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
35s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
54s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
54s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
54s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
37s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} whitespace {color} | {color:red} 0m
0s{color} | {color:red} The patch has 1 line(s) that end in whitespace. Use git
apply --whitespace=fix <>. Refer https://git-scm.com/docs/git-apply
{color} |
| {color:red}-1{color} | {color:red} whitespace {color} | {color:red} 0m
0s{color} | {color:red} The patch 1 line(s) with tabs. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
11m 34s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 19m 18s{color}
| {color:red} hadoop-yarn-server-nodemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
23s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 72m 39s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.nodemanager.containermanager.scheduler.TestContainerSchedulerQueuing
|
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12922152/YARN-8207.006.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux cc1bd3888175 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9
14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / a732acd |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| whitespace |
https://builds.apache.org/job/PreCommit-YARN-Build/20606/artifact/out/whitespace-eol.txt
|
| whitespace |
https://builds.apache.org/job/PreCommit-YARN-Build/20606/artifact/out/whitespace-tabs.txt
|
| unit |
https://builds.apache.org/job/PreCommit-YARN-Build/20606/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20606/testReport/ |
| Max. process+thread count | 303 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20606/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> Docker container launch use popen have risk of
[jira] [Commented] (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=16464873#comment-16464873 ] Eric Yang commented on YARN-8207: - [~jlowe] Patch 006 contains all style fixes from your recommendations. > 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, > YARN-8207.006.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] [Commented] (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=16464035#comment-16464035 ] Eric Yang commented on YARN-8207: - [~jlowe] I see your concerns now. Thanks for the explanation. I will update the code to use typedef data structure above, and ensure null terminator is passed to execvp after getting the data out of args. > 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] [Commented] (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=16463950#comment-16463950 ] Jason Lowe commented on YARN-8207: -- bq. Args is array of strings. Null terminator is not required for array when we have length of the array. construct_docker_command may use an args structure internally, but it simply returns a char** as a result with no specified length. The code then passes that to execvp which needs the NULL pointer terminator to know when the argument list ends (see the execvp manpage for details). bq. Hence checking length > DOCKER_ARGS_MAX is fine. No, it's not. It's an off-by-one error. reset_args only allocates DOCKER_ARGS_MAX elements in the array. add_to_args checks for out-of-bounds by checking index > DOCKER_ARGS_MAX. If index == DOCKER_ARGS_MAX then the bounds check will pass and the code will assign a value to out[DOCKER_ARGS_MAX]. That store will then corrupt the heap as the memory allocated only has space for DOCKER_ARGS_MAX elements in the array. > 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] [Commented] (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=16463266#comment-16463266
]
Eric Yang commented on YARN-8207:
-
[~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] [Commented] (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=16463048#comment-16463048
]
Jason Lowe commented on YARN-8207:
--
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.
> Docker container launch use popen have risk of shell expansion
>
[jira] [Commented] (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=16460387#comment-16460387
]
genericqa commented on YARN-8207:
-
| (/) *{color:green}+1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
23s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 23m
4s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
50s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
35s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
33m 57s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
32s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
46s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
46s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
46s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
31s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
10m 29s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 19m
37s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
23s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 67m 10s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12921498/YARN-8207.005.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 53a8ab785232 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20
11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 8f42daf |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20561/testReport/ |
| Max. process+thread count | 410 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20561/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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,
> 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
[jira] [Commented] (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=16460340#comment-16460340
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
23s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 26m
33s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
53s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
35s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
38m 6s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
50s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
56s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} cc {color} | {color:red} 0m 56s{color} |
{color:red}
hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager
generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
56s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
32s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
11m 52s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 19m
17s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
22s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 72m 49s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12921481/YARN-8207.004.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux f6006dc95f21 3.13.0-137-generic #186-Ubuntu SMP Mon Dec 4
19:09:19 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 8f42daf |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| cc |
https://builds.apache.org/job/PreCommit-YARN-Build/20560/artifact/out/diff-compile-cc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20560/testReport/ |
| Max. process+thread count | 341 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20560/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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,
[jira] [Commented] (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=16460297#comment-16460297 ] Eric Yang commented on YARN-8207: - Patch 005 removed bug found by cc. > 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, > 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] [Commented] (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=16460255#comment-16460255 ] Eric Yang commented on YARN-8207: - [~Jim_Brennan] Thank you for the review. I made the changes according to your comments in patch 004. For the last part, fixed array is safer than dynamic one. The original patch in YARN-7654 was mostly static allocation. I think it is safer to use static size for args to prevent any chance of stack overflow by dynamic allocation of large string array. Make_string was suggested by Jason to improve code readability. > 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, > YARN-8207.003.patch, YARN-8207.004.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] [Commented] (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=16460139#comment-16460139 ] Jim Brennan commented on YARN-8207: --- [~eyang], I spent a little time looking at this one today. Didn't get too far, but here are few comments. docker-util.c: * (nit) add_to_buffer() funxction name doesn't seem to fit anymore, maybe add_to_arglist()? * free_buffer() - should set argv->out[i] = NULL after freeing. * (nit) struct argv is a confusing name because of the standard main() char **argv param - maybe struct arglist, or something like that? * get_docker_volume_command() ** looks like 'docker' is never freed. ** (nit) make_string() is a nice abstraction, but all those little alloc/frees seems expensive. ** (nit) add_mounts() - you can put the free(mount_src) at the end inside an if !NULL instead of calling it in every condition. ** reset() is a very generic name for a global function - also if this is limited to DOCKER_ARG_MAX entries, why isn't it just a fixed array of char*? If you are going to malloc it, why not support auto-resizing of it? > 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, > YARN-8207.003.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] [Commented] (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=16460098#comment-16460098
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
15s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 26m
47s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
58s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
41s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
38m 27s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
38s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
51s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} cc {color} | {color:red} 0m 51s{color} |
{color:red}
hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager
generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
51s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
33s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
11m 23s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 18m 55s{color}
| {color:red} hadoop-yarn-server-nodemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
24s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 71m 58s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.nodemanager.containermanager.scheduler.TestContainerSchedulerQueuing
|
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12921441/YARN-8207.003.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux 7a4615ca8e02 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9
14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 9e2cfb2 |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| cc |
https://builds.apache.org/job/PreCommit-YARN-Build/20555/artifact/out/diff-compile-cc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| unit |
https://builds.apache.org/job/PreCommit-YARN-Build/20555/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20555/testReport/ |
| Max. process+thread count | 302 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20555/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> Docker container launch use popen have risk of shell expansion
> --
>
> Key: YARN-8207
> URL:
[jira] [Commented] (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=16460010#comment-16460010 ] Eric Yang commented on YARN-8207: - Patch 003 restored the blocking and wait for child pid behavior, and fixed make_string method. > 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, > YARN-8207.003.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] [Commented] (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=16459992#comment-16459992
]
Eric Yang commented on YARN-8207:
-
{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] [Commented] (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=16459923#comment-16459923 ] Jason Lowe commented on YARN-8207: -- bq. If docker run is aborted prior to complete download, it does not leave partial image or container instance behind. 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. bq. The original while loop repeats for every character that doesn't fit in the buffer size for string greater than 100 characters. By cut down the number of retries of the loop by half using twos increment of buffer size. 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. bq. and to avoid possible copyright issue for borrowing code from GPL man page. Lifting code verbatim, whitespace formatting and all, and changing a single character in a method is not going to avoid copyright/GPL concerns. If that is the main concern then we should just write our own version from scratch based on how vsnprintf works. [One example is in a comment|https://issues.apache.org/jira/browse/YARN-7654?focusedCommentId=16420623=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16420623] I made in YARN-7654 which has no looping and simply calls vsnprintf twice, once without any buffer to determine the required buffer size and once more to finally sprintf the string with the properly sized buffer. > 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] [Commented] (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=16459839#comment-16459839
]
Eric Yang commented on YARN-8207:
-
{quote}One problem with the foreground change being implemented here is when
the docker image needs to be downloaded. The inspect command will fail until
the docker container starts running, but the container will not start running
until the necessary layers are downloaded. If this takes more than 10 seconds
(or whatever the retries are) then it will report a failed launch yet the
Docker container will eventually start when the layers finish downloading. At
that point I believe the NM will have leaked a Docker container.{quote}
No leak of docker container. Download happens before docker daemon registered
creation of docker container. If docker run is aborted prior to complete
download, it does not leave partial image or container instance behind.
{quote}For the purposes of this JIRA, I think we should preserve the current
behavior (i.e.: wait for the run command to return then do the inspect).{quote}
I will update the code to preserve detach and wait for this JIRA.
{quote}Cool, was this measured empirically or referenced from some article? It
would be good to put a comment explaining the +2, otherwise it's likely to get
"fixed" by someone coming along later and thinking it's an off-by-one error.
Also I'm not sure what's meant by "recursion" here since the function is not
recursive. Could you explain?{quote}
Sorry recursion is the wrong word. The original while loop repeats for every
character that doesn't fit in the buffer size for string greater than 100
characters. By cut down the number of retries of the loop by half using twos
increment of buffer size.
It reduces the time spent in the loop in half for strings that goes beyond 120
characters. This is more empirically optimization, and to avoid possible
copyright issue for borrowing code from GPL man page.
> 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] [Commented] (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=16459276#comment-16459276
]
genericqa commented on YARN-8207:
-
| (/) *{color:green}+1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
23s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 22m
57s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
47s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
34s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
33m 45s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
32s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
48s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
48s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
48s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
30s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
10m 24s{color} | {color:green} patch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 19m
34s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed.
{color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
25s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 66m 52s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12921321/YARN-8207.002.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux f1061898eff5 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20
11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / fc074a3 |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20542/testReport/ |
| Max. process+thread count | 410 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20542/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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
[jira] [Commented] (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=16459207#comment-16459207
]
Jason Lowe commented on YARN-8207:
--
Thanks for updating the patch! I will try to take a look at it tomorrow.
{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}
If the fopen and fclose are not doing anything useful until YARN-7654 then
those changes should be deferred to that JIRA.
{quote}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.
{quote}
If we need the detached docker run progress to be in the user log area for
debugging that is easy to setup, just like it is being setup here for the
foreground run. Running in detached mode does not preclude getting progress if
we want it.
One problem with the foreground change being implemented here is when the
docker image needs to be downloaded. The inspect command will fail until the
docker container starts running, but the container will not start running until
the necessary layers are downloaded. If this takes more than 10 seconds (or
whatever the retries are) then it will report a failed launch yet the Docker
container will eventually start when the layers finish downloading. At that
point I believe the NM will have leaked a Docker container.
One advantage of running in detached mode is that the run command will not
return until the container starts running. That way we know there's no point in
trying to call inspect until the run command returns, and it also will wait
indefinitely for the image to download. For the purposes of this JIRA, I think
we should preserve the current behavior (i.e.: wait for the run command to
return then do the inspect). We can discuss changing to foreground behavior in
YARN-7654, but changing to foreground is not relevant for this JIRA.
{quote}This change makes make_string function twice faster than sample code
while waste 1% or less space if recursion is required.
{quote}
Cool, was this measured empirically or referenced from some article? It would
be good to put a comment explaining the +2, otherwise it's likely to get
"fixed" by someone coming along later and thinking it's an off-by-one error.
Also I'm not sure what's meant by "recursion" here since the function is not
recursive. Could you explain?
> 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] [Commented] (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=16459178#comment-16459178 ] Eric Yang commented on YARN-8207: - [~jlowe] Patch 002 has converted to use struct instead of char **out, int *index combination, and addressed coding style issues that you mentioned above. > 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] [Commented] (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=16457139#comment-16457139
]
Eric Yang commented on YARN-8207:
-
[~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: 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
>
>
> 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] [Commented] (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=16457085#comment-16457085
]
Jason Lowe commented on YARN-8207:
--
Thanks for the patch!
Nit: chosen_container_log_dir should be static.
init_log_path should use make_string instead of manually calculating string
lengths.
It would make the code easier to read and maintain if the argv pointer array
and size of the array (i.e. \*\*out and \*index) were passed around as a
structure instead of separate function arguments. It's easier to treat them
together as a single object. For example, it would be simple for free_buffer
to not only free all the arguments but also reset the size to zero at the same
time since it has access to the whole structure.
DOCKER_ARG_MAX is sometimes used as a maximum argument count and other times
used as a character buffer count which is inconsistent. For example, the
following code has nothing to do with the maximum argument count for the docker
command. It should be using make_string to form the command rather than trying
to manually size the buffer.
{code}
size_t command_size = DOCKER_ARG_MAX;
char* proc_pid_path = alloc_and_clear_memory(command_size, sizeof(char));
snprintf(proc_pid_path, command_size, "%s/%d", PROC_PATH, pid);
{code}
docker_command_with_binary is initialized to an allocated buffer and then
overwritten with the result of the call to flatten.
docker_wait_command and docker_rm_command are used to track an allocated buffer
that is never used.
docker_inspect_command and docker_inspect_exitcode_command are tracking a
DOCKER_ARG_MAX buffer used for popen. Should these be using execv instead of
popen as part of this change? If not, DOCKER_ARG_MAX isn't an appropriate
buffer sizer for those per above discussion. I think we can simply use
make_string for those.
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?
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.
Nit: flatten would be more efficient and quite a bit simpler if written with
stpcpy rather than strlen and strncpy.
add_param_to_command and add_param_to_command_if_allowed should use make_string
instead of guessing at the required buffer size.
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 is get_docker_inspect_command calling free_buffer at the beginning? Index
is left unmodified and we're not nulling the values after freeing them in
free_buffer, so I'm not sure what the intent is with that call here. I think
the method should simply append to the buffer, and it is the responsibility of
the caller to make sure the buffer is in the desired state before calling the
function that will append the commands. Similar comment for other places
free_buffer is called towards the beginning of a function.
The set_env changes look relevant to YARN-7654 rather than this JIRA.
get_docker_run_command should use make_string.
Why does make_string calculate size = n + 2 instead of n + 1?
Style nit: space between realloc and parentheses in make_string
I didn't quite get through everything today, and I'll try to get to the rest of
it early next week.
> 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
>
>
> 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
[jira] [Commented] (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=16456908#comment-16456908
]
genericqa commented on YARN-8207:
-
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
23s{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} ||
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 1 new or modified test
files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 22m
47s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
46s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
31s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
32m 30s{color} | {color:green} branch has no errors when building and testing
our client artifacts. {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
29s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
29s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
9m 38s{color} | {color:green} patch has no errors when building and testing our
client artifacts. {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 20m 1s{color}
| {color:red} hadoop-yarn-server-nodemanager in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
20s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 65m 2s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests |
hadoop.yarn.server.nodemanager.containermanager.TestContainerManager |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:abb62dd |
| JIRA Issue | YARN-8207 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12920712/YARN-8207.001.patch |
| Optional Tests | asflicense compile cc mvnsite javac unit |
| uname | Linux e940a8bc6d43 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20
11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / ef3ecc3 |
| maven | version: Apache Maven 3.3.9 |
| Default Java | 1.8.0_162 |
| unit |
https://builds.apache.org/job/PreCommit-YARN-Build/20514/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-YARN-Build/20514/testReport/ |
| Max. process+thread count | 440 (vs. ulimit of 1) |
| modules | C:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
U:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
|
| Console output |
https://builds.apache.org/job/PreCommit-YARN-Build/20514/console |
| Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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
>
>
> Container-executor code
[jira] [Commented] (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=1645#comment-1645 ] Eric Yang commented on YARN-8207: - Extract logic from YARN-7654 patch 17 for using execvp for launching docker container. > 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 > > > 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
