From f5568e15d0d6fdf9593ee869485aa6471b889d52 Mon Sep 17 00:00:00 2001 From: Hideyuki Furue Date: Wed, 2 Jun 2021 19:56:38 +0900 Subject: [PATCH] Fix container-executor Signed-off-by: Akira Ajisaka (cherry picked from commit 9c7b8cf54ea88833d54fc71a9612c448dc0eb78d) --- .../impl/container-executor.c | 118 ++++++++++++++---- .../native/container-executor/impl/main.c | 4 +- .../native/container-executor/impl/util.c | 10 +- .../impl/utils/string-utils.c | 1 + .../test/test-container-executor.c | 41 ++++++ 5 files changed, 144 insertions(+), 30 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c index f68308c54b0..693b731fc67 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c @@ -47,6 +47,10 @@ #include #include #include +#ifdef __linux +#include +#include +#endif #ifndef HAVE_FCHMODAT #include "compat/fchmodat.h" @@ -222,6 +226,19 @@ static int write_pid_to_cgroup_as_root(const char* cgroup_file, pid_t pid) { goto cleanup; } + // statfs + struct statfs buf; + if (statfs(cgroup_file, &buf) == -1) { + fprintf(LOGFILE, "Can't statfs file %s as node manager - %s\n", cgroup_file, + strerror(errno)); + rc = -1; + goto cleanup; + } else if (buf.f_type != CGROUP_SUPER_MAGIC) { + fprintf(LOGFILE, "Pid file %s is not located on cgroup filesystem\n", cgroup_file); + rc = -1; + goto cleanup; + } + // open int cgroup_fd = open(cgroup_file, O_WRONLY | O_APPEND, 0); if (cgroup_fd == -1) { @@ -499,27 +516,16 @@ int is_mount_cgroups_support_enabled() { /** * Utility function to concatenate argB to argA using the concat_pattern. + * For historical reasons, redundant argument numArgs exists. */ char *concatenate(char *concat_pattern, char *return_path_name, int numArgs, ...) { va_list ap; va_start(ap, numArgs); - int strlen_args = 0; - char *arg = NULL; - int j; - for (j = 0; j < numArgs; j++) { - arg = va_arg(ap, char*); - if (arg == NULL) { - fprintf(LOGFILE, "One of the arguments passed for %s is null.\n", - return_path_name); - return NULL; - } - strlen_args += strlen(arg); - } + int str_len = vsnprintf(NULL, 0, concat_pattern, ap) + 1; va_end(ap); char *return_path = NULL; - int str_len = strlen(concat_pattern) + strlen_args + 1; return_path = (char *) malloc(str_len); if (return_path == NULL) { @@ -736,6 +742,10 @@ static int create_container_directories(const char* user, const char *app_id, if (container_dir == NULL) { return OUT_OF_MEMORY; } + if (strstr(container_dir, "..") != 0) { + fprintf(LOGFILE, "Unsupported container directory path detected.\n"); + return COULD_NOT_CREATE_WORK_DIRECTORIES; + } if (mkdirs(container_dir, perms) == 0) { result = 0; } @@ -763,19 +773,26 @@ static int create_container_directories(const char* user, const char *app_id, char *container_log_dir = get_app_log_directory(*log_dir_ptr, combined_name); int check = check_nm_local_dir(nm_uid, *log_dir_ptr); if (check != 0) { - container_log_dir = NULL; - } - if (strstr(container_log_dir, "..") != 0) { - fprintf(LOGFILE, "Unsupported container log directory path detected.\n"); - container_log_dir = NULL; + free(container_log_dir); + free(combined_name); + return COULD_NOT_CREATE_APP_LOG_DIRECTORIES; } if (container_log_dir == NULL) { free(combined_name); return OUT_OF_MEMORY; + } + if (strstr(container_log_dir, "..") != 0) { + fprintf(LOGFILE, "Unsupported container log directory path detected.\n"); + free(container_log_dir); + free(combined_name); + return COULD_NOT_CREATE_APP_LOG_DIRECTORIES; } else if (mkdirs(container_log_dir, logdir_perms) != 0) { free(container_log_dir); } else { result = 0; + if (chosen_container_log_dir != NULL) { + free(chosen_container_log_dir); + } chosen_container_log_dir = strdup(container_log_dir); free(container_log_dir); } @@ -794,9 +811,18 @@ static int create_container_directories(const char* user, const char *app_id, if (tmp_dir == NULL) { return OUT_OF_MEMORY; } + + if (strstr(tmp_dir, "..") != 0) { + fprintf(ERRORFILE, "Unsupported tmp directory path detected.\n"); + result = COULD_NOT_CREATE_TMP_DIRECTORIES; + goto cleanup; + } + if (mkdirs(tmp_dir, perms) == 0) { result = 0; } + +cleanup: free(tmp_dir); return result; @@ -970,7 +996,7 @@ static int change_owner(const char* path, uid_t user, gid_t group) { * return non-0 on failure */ int create_directory_for_user(const char* path) { - // set 2750 permissions and group sticky bit + // set 750 permissions and setgid bit mode_t permissions = S_IRWXU | S_IRGRP | S_IXGRP | S_ISGID; uid_t user = geteuid(); gid_t group = getegid(); @@ -983,13 +1009,13 @@ int create_directory_for_user(const char* path) { if (ret == 0) { if (0 == mkdir(path, permissions) || EEXIST == errno) { - // need to reassert the group sticky bit + // need to reassert the setgid bit if (change_owner(path, user, nm_gid) != 0) { fprintf(LOGFILE, "Failed to chown %s to %d:%d: %s\n", path, user, nm_gid, strerror(errno)); ret = -1; } else if (chmod(path, permissions) != 0) { - fprintf(LOGFILE, "Can't chmod %s to add the sticky bit - %s\n", + fprintf(LOGFILE, "Can't chmod %s to add the setgid bit - %s\n", path, strerror(errno)); ret = -1; } @@ -1099,6 +1125,11 @@ int initialize_user(const char *user, char* const* local_dirs) { fprintf(LOGFILE, "Couldn't get userdir directory for %s.\n", user); failed = 1; break; + // Avoid possible wrong validation. Username can contain double dots. + } else if (strstr(user_dir, "/../") != 0) { + fprintf(LOGFILE, "Unsupported userdir directory path detected.\n"); + failed = 1; + break; } if (create_directory_for_user(user_dir) != 0) { failed = 1; @@ -1120,6 +1151,9 @@ int create_log_dirs(const char *app_id, char * const * log_dirs) { } if (app_log_dir == NULL) { // try the next one + } else if (strstr(app_log_dir, "..") != 0) { + fprintf(LOGFILE, "Unsupported app-log directory path detected.\n"); + free(app_log_dir); } else if (create_directory_for_user(app_log_dir) != 0) { free(app_log_dir); return -1; @@ -1188,7 +1222,11 @@ int create_container_log_dirs(const char *container_id, const char *app_id, } int result = check_nm_local_dir(nm_uid, *log_root); - if (result != 0 && container_log_dir != NULL) { + if (result != 0) { + free(container_log_dir); + container_log_dir = NULL; + continue; + } else if (strstr(container_log_dir, "..") != 0) { fprintf(LOGFILE, "Unsupported container log directory path (%s) detected.\n", container_log_dir); free(container_log_dir); @@ -1233,6 +1271,9 @@ static char *create_app_dirs(const char *user, char *app_dir = get_app_directory(*nm_root, user, app_id); if (app_dir == NULL) { // try the next one + } else if (strstr(app_dir, "..") != 0) { + fprintf(LOGFILE, "Unsupported app directory path detected.\n"); + free(app_dir); } else if (mkdirs(app_dir, permissions) != 0) { free(app_dir); } else if (primary_app_dir == NULL) { @@ -1299,7 +1340,7 @@ int initialize_app(const char *user, const char *app_id, char *nmPrivate_credentials_file_copy = strdup(nmPrivate_credentials_file); // TODO: FIXME. The user's copy of creds should go to a path selected by - // localDirAllocatoir + // localDirAllocator char *cred_file_name = concatenate("%s/%s", "cred file", 2, primary_app_dir, basename(nmPrivate_credentials_file_copy)); if (cred_file_name == NULL) { @@ -1406,14 +1447,14 @@ int create_script_paths(const char *work_dir, int exit_code = -1; *script_file_dest = get_container_launcher_file(work_dir); - if (script_file_dest == NULL) { + if (*script_file_dest == NULL) { exit_code = OUT_OF_MEMORY; fprintf(ERRORFILE, "Could not create script_file_dest\n"); return exit_code; } *cred_file_dest = get_container_credentials_file(work_dir); - if (NULL == cred_file_dest) { + if (NULL == *cred_file_dest) { exit_code = OUT_OF_MEMORY; fprintf(ERRORFILE, "Could not create cred_file_dest\n"); return exit_code; @@ -1526,6 +1567,12 @@ int create_user_filecache_dirs(const char * user, char* const* local_dirs) { rc = INITIALIZE_USER_FAILED; break; } + if (strstr(filecache_dir, "..") != 0) { + fprintf(LOGFILE, "Unsupported filecache directory path detected.\n"); + free(filecache_dir); + rc = INITIALIZE_USER_FAILED; + break; + } if (0 != mkdir(filecache_dir, permissions) && EEXIST != errno) { fprintf(LOGFILE, "Failed to create directory %s - %s\n", filecache_dir, strerror(errno)); @@ -1616,6 +1663,12 @@ int launch_docker_container_as_user(const char * user, const char *app_id, docker_command_with_binary = flatten(docker_command); + if (docker_command_with_binary == NULL) { + fprintf (ERRORFILE, "Could not flatten docker command.\n"); + exit_code = UNABLE_TO_EXECUTE_CONTAINER_SCRIPT; + goto cleanup; + } + // Launch container pid_t child_pid = fork(); if (child_pid == -1) { @@ -2315,6 +2368,7 @@ int list_as_user(const char *target_dir) { strerror(errno)); ret = -1; } + closedir(dir); } else { fprintf(LOGFILE, "Could not open directory %s - %s\n", target_dir, strerror(errno)); @@ -2372,8 +2426,10 @@ int is_empty(char *target_dir) { continue; } fprintf(LOGFILE, "Directory is not empty %s\n", target_dir); + closedir(dir); return 0; } + closedir(dir); return 1; } @@ -2400,7 +2456,7 @@ int mount_cgroup(const char *pair, const char *hierarchy) { goto cleanup; } if (hierarchy == NULL || strstr(hierarchy, "..") != NULL) { - fprintf(LOGFILE, "Unsupported cgroup hierarhy path detected.\n"); + fprintf(LOGFILE, "Unsupported cgroup hierarchy path detected.\n"); result = INVALID_COMMAND_PROVIDED; goto cleanup; } @@ -2421,8 +2477,13 @@ int mount_cgroup(const char *pair, const char *hierarchy) { result = INVALID_COMMAND_PROVIDED; goto cleanup; } + if (strlen(mount_path) + strlen(hierarchy) + 2 > EXECUTOR_PATH_MAX) { + fprintf(LOGFILE, "cgroup hierarchy path is too long.\n"); + result = INVALID_COMMAND_PROVIDED; + goto cleanup; + } if (mount("none", mount_path, "cgroup", 0, controller) == 0) { - char *buf = stpncpy(hier_path, mount_path, strlen(mount_path)); + char *buf = stpncpy(hier_path, mount_path, EXECUTOR_PATH_MAX); *buf++ = '/'; snprintf(buf, EXECUTOR_PATH_MAX - (buf - hier_path), "%s", hierarchy); @@ -2535,6 +2596,9 @@ char* flatten(char **args) { total = total + strlen(args[i]) + 1; } char *buffer = (char *) malloc(total * sizeof(char)); + if (buffer == NULL) { + return NULL; + } char *to = NULL; to = buffer; for (int i = 0; args[i] != NULL; i++) { diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c index 603b2d7cb33..5624966cd93 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c @@ -69,8 +69,8 @@ static void display_usage(FILE *stream) { fprintf(stream, " container-executor \n" " where command and command-args: \n" \ - " initialize container: %2d appid tokens nm-local-dirs " - "nm-log-dirs cmd app...\n" + " initialize container: %2d appid containerid tokens nm-local-dirs " + "nm-log-dirs cmd...\n" " launch container: %2d appid containerid workdir " "container-script tokens pidfile nm-local-dirs nm-log-dirs resources ", INITIALIZE_CONTAINER, LAUNCH_CONTAINER); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/util.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/util.c index ba05adcea1a..fffa7143302 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/util.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/util.c @@ -25,6 +25,7 @@ char** split_delimiter(char *value, const char *delim) { char **return_values = NULL; + char **new_return_values; char *temp_tok = NULL; char *tempstr = NULL; int size = 0; @@ -57,8 +58,15 @@ char** split_delimiter(char *value, const char *delim) { // Make sure returned values has enough space for the trailing NULL. if (size >= return_values_size - 1) { return_values_size += per_alloc_size; - return_values = (char **) realloc(return_values,(sizeof(char *) * + new_return_values = (char **) realloc(return_values,(sizeof(char *) * return_values_size)); + if (!new_return_values) { + fprintf(ERRORFILE, "Reallocation error for return_values in %s.\n", + __func__); + failed = 1; + goto cleanup; + } + return_values = new_return_values; // Make sure new added memory are filled with NULL for (int i = size; i < return_values_size; i++) { diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.c index 80511e573ed..1258e202f0c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/string-utils.c @@ -175,6 +175,7 @@ char *make_string(const char *fmt, ...) { int ret = vsnprintf(buf, buflen, fmt, vargs); va_end(vargs); if (ret < 0) { + free(buf); buf = NULL; } } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c index 6f5b8849f48..62c3af9ad28 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/test-container-executor.c @@ -1218,6 +1218,46 @@ void test_trim_function() { free(trimmed); } +/** + * This test is used to verify that concatenate() works correctly + */ +void test_concatenate() { + char *concatenate(char *concat_pattern, char *return_path_name, int numArgs, ...); + printf("\nTesting concatenate function\n"); + + // numArgs: 0 + char *expected1 = "fixed1"; + char *actual1 = concatenate("fixed1", "test1", 0); + if (actual1 == NULL || strcmp(actual1, expected1) != 0) { + printf("FAIL: concatenate: test1: expected %s got %s\n", expected1, actual1); + exit(1); + } + + // numArgs: 1 + char *expected2 = "fixed1/var1"; + char *actual2 = concatenate("fixed1/%s", "test2", 1, "var1"); + if (actual2 == NULL || strcmp(actual2, expected2) != 0) { + printf("FAIL: concatenate: test2: expected %s got %s\n", expected2, actual2); + exit(1); + } + + // numArgs: 2 + char *expected3 = "fixed1/var1/fixed2/var2"; + char *actual3 = concatenate("fixed1/%s/fixed2/%s", "test3", 2, "var1", "var2"); + if (actual3 == NULL || strcmp(actual3, expected3) != 0) { + printf("FAIL: concatenate: test3: expected %s got %s\n", expected3, actual3); + exit(1); + } + + // concat_pattern with field width + char *expected4 = "[x ]"; + char *actual4 = concatenate("[%-10s]", "test4", 1, "x"); + if (actual4 == NULL || strcmp(actual4, expected4) != 0) { + printf("FAIL: concatenate: test4: expected %s got %s\n", expected4, actual4); + exit(1); + } +} + int is_empty(char *name); void test_is_empty() { @@ -1570,6 +1610,7 @@ int main(int argc, char **argv) { #endif test_trim_function(); + test_concatenate(); printf("\nFinished tests\n"); free(current_username);