[
https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Varun Vasudev updated YARN-6623:
--------------------------------
Attachment: YARN-6623.004.patch
Thanks for the review [~miklos.szeg...@cloudera.com]!
{quote}
/proc/mounts,/sys/fs/cgroup are always in the same place
This is actually not completely true. If you run in a container,/sys/fs/cgroup
can be anywhere
490 .addReadOnlyMountLocation(CGROUPS_ROOT_DIRECTORY,
491 CGROUPS_ROOT_DIRECTORY, false);
This is actually a security issue. As opposed to lxcfs, mounting cgroups will
expose information about all the other containers to each container. This
change is big enough but you might want to whitelist this in the future.
{quote}
This was already being set before this patch. Perhaps, it can be handled
cleanly as part of YARN-5534?
{quote}
SET(CMAKE_CXX_FLAGS "$\{CMAKE_CXX_FLAGS} -Wall -std=c++11")
Not sure, but this might not build on old OS like centos6
{quote}
Fixed.
{quote}
76 printWriter.println("\[docker-command-execution]");
77 for (Map.Entry<String, List<String>> entry :
cmd.getCommandArguments()
78 .entrySet()) \{
79 printWriter.println(" " + entry.getKey() + "=" + StringUtils
80 .join(",", entry.getValue()));
81 }
Probably it does worth to check, if entry for example contains “abc=\ndef” to
avoid injection attacks.
{quote}
Fixed.
{quote}
169 ret\[i + 1] = '\0';
I think it is safe to do a single ret\[I] = 0; after the loop
{quote}
Fixed.
{quote}
177 void quote_and_append_arg(char **str, size_t *size, const char* param,
const char *arg) \{
178 char *tmp = escape_single_quote(arg);
179 strcat(*str, param);
180 strcat(*str, "'");
181 if(strlen(*str) + strlen(tmp) > *size) \{
182 *str = (char *) realloc(*str, strlen(*str) + strlen(tmp) + 1024);
183 if(*str == NULL) \{
184 exit(OUT_OF_MEMORY);
185 }
186 *size = strlen(*str) + strlen(tmp) + 1024;
187 }
188 strcat(*str, tmp);
189 strcat(*str, "' ");
190 free(tmp);
191 }
What is 1024? How about setting * size before *str, so that there is no need
for duplication?
{quote}
Fixed.
{quote}
#define EXECUTOR_PATH_MAX 131072
This is lots of allocation. The OS actually needs to zero all the allocated
memory before giving it to other processes after close, so this might add to
the overall CPU usage and memory bandwidth.
{quote}
Fixed. Set back to 4096.
{quote}
35 if (command != NULL) \{
36 free(command);
37 }
This is not necessary, free always ignores NULL argument.
{quote}
Fixed.
{quote}
47 if (current_len + string_len < bufflen) \{
bufflen-1 to allocate space for the terminating 0
{quote}
Fixed.
{quote}
48 strncpy(buff + current_len, string, bufflen - current_len);
strncpy does not add 0 terminator at the end of the target, if
strlen(string)==bufflen - current_len resulting in a read buffer overflow later.
{quote}
Fixed.
{quote}
165 if (0 == strncmp(container_name, CONTAINER_NAME_PREFIX,
strlen(CONTAINER_NAME_PREFIX))) \{
This is a double scan. You can just use strcmp with the same effect.
{quote}
{quote}
249 const char *regex_str =
"^((\[a-zA-Z0-9.-]+)(:\[0-9]+)?/)?(\[a-z0-9_./-]+)(:\[a-zA-Z0-9_.-]+)?$”;
Image can be specified by a digest, which is more secure. I do not see that
supported by the regex. IMAGE\[:TAG|@DIGEST]
{quote}
[~shaneku...@gmail.com] added this, I'm just moving it to a different file.
Shane can you please comment?
{quote}
# docker.binary=/bin/docker
115 docker_binary = strdup("/usr/bin/docker”);
We should choose one and use it everywhere.
{quote}
Fixed.
{quote}
477 permitted_mount_len = strlen(permitted_mounts\[i]);
478 if (permitted_mounts\[i]\[permitted_mount_len - 1] == '/') \{
Buffer underflow at permitted_mount_len == 0
{quote}
Fixed.
{quote}
488 static char* normalize_mount(const char* mount) \{
It would be really nice to have some comments here.
{quote}
Fixed.
{quote}
503 size_t len = strlen(real_mount);
504 if (real_mount\[len - 1] != '/') {
505 ret_ptr = (char *) alloc_and_clear_memory(len + 2, sizeof(char));
506 strncpy(ret_ptr, real_mount, len);
507 ret_ptr\[len] = '/';
508 ret_ptr\[len + 1] = '\0';
Potential buffer underflow moreover most likely the character does not match
and we end with a normalized mount path of “/“. This happens, when
strlen(real_mount)==0
{quote}
Fixed
> Add support to turn off launching privileged containers in the
> container-executor
> ---------------------------------------------------------------------------------
>
> Key: YARN-6623
> URL: https://issues.apache.org/jira/browse/YARN-6623
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Reporter: Varun Vasudev
> Assignee: Varun Vasudev
> Priority: Blocker
> Attachments: YARN-6623.001.patch, YARN-6623.002.patch,
> YARN-6623.003.patch, YARN-6623.004.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should
> add a flag to the container-executor.cfg allowing admins to disable launching
> privileged containers at the container-executor level.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
