From 14bf55d49aafb4bc28c9249c411f17a85c4000e7 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 18 Aug 2017 16:22:52 +0200 Subject: [PATCH 1/2] Fix cgroup mountinfo parsing The parsing would find the wrong '-' in lines like this: 354 347 0:28 /system.slice/docker-654dd7b6b8bbfe1739ae3309b471e95ccc82b3a3f56b7879f0a811d68b5c4e1d.scope /sys/fs/cgroup/cpuacct,cpu ro,nosuid,nodev,noexec,relatime - cgroup cgroup rw,cpuacct,cpu --- src/gc/unix/cgroup.cpp | 4 ++-- src/pal/src/misc/cgroup.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gc/unix/cgroup.cpp b/src/gc/unix/cgroup.cpp index 4721f09f86b2..85a12bb2184d 100644 --- a/src/gc/unix/cgroup.cpp +++ b/src/gc/unix/cgroup.cpp @@ -189,11 +189,11 @@ class CGroup maxLineLen = lineLen; } - char* separatorChar = strchr(line, '-'); + char* separatorChar = strstr(line, " - "); // See man page of proc to get format for /proc/self/mountinfo file int sscanfRet = sscanf(separatorChar, - "- %s %*s %s", + " - %s %*s %s", filesystemType, options); if (sscanfRet != 2) diff --git a/src/pal/src/misc/cgroup.cpp b/src/pal/src/misc/cgroup.cpp index 46d83842defa..84e56ac55d23 100644 --- a/src/pal/src/misc/cgroup.cpp +++ b/src/pal/src/misc/cgroup.cpp @@ -189,11 +189,11 @@ class CGroup goto done; maxLineLen = lineLen; } - char* separatorChar = strchr(line, '-'); + char* separatorChar = strstr(line, " - ");; // See man page of proc to get format for /proc/self/mountinfo file int sscanfRet = sscanf_s(separatorChar, - "- %s %*s %s", + " - %s %*s %s", filesystemType, lineLen+1, options, lineLen+1); if (sscanfRet != 2) From 43dd70ce2e79d4c85028cb7f051d2ed6c65bd1c4 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 21 Aug 2017 09:48:29 +0200 Subject: [PATCH 2/2] cgroup: don't append cgroup relative path for reading docker limits --- src/gc/unix/cgroup.cpp | 88 +++++++++++++++------------------- src/pal/src/misc/cgroup.cpp | 95 +++++++++++++++---------------------- 2 files changed, 76 insertions(+), 107 deletions(-) diff --git a/src/gc/unix/cgroup.cpp b/src/gc/unix/cgroup.cpp index 85a12bb2184d..992678b634f8 100644 --- a/src/gc/unix/cgroup.cpp +++ b/src/gc/unix/cgroup.cpp @@ -36,8 +36,8 @@ class CGroup public: CGroup() { - m_memory_cgroup_path = FindMemoryCgroupPath(); - m_cpu_cgroup_path = FindCpuCgroupPath(); + m_memory_cgroup_path = FindCgroupPath(&IsMemorySubsystem); + m_cpu_cgroup_path = FindCgroupPath(&IsCpuSubsystem); } ~CGroup() @@ -110,65 +110,45 @@ class CGroup return strcmp("cpu", strTok) == 0; } - static char* FindMemoryCgroupPath(){ - char *memory_cgroup_path = nullptr; - char *memory_hierarchy_mount = nullptr; - char *mem_cgroup_path_relative_to_mount = nullptr; - - memory_hierarchy_mount = FindHierarchyMount(&IsMemorySubsystem); - if (memory_hierarchy_mount == nullptr) - goto done; - - mem_cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(&IsMemorySubsystem); - if (mem_cgroup_path_relative_to_mount == nullptr) - goto done; - - memory_cgroup_path = (char*)malloc(strlen(memory_hierarchy_mount) + strlen(mem_cgroup_path_relative_to_mount) + 1); - if (memory_cgroup_path == nullptr) - goto done; - - strcpy(memory_cgroup_path, memory_hierarchy_mount); - strcat(memory_cgroup_path, mem_cgroup_path_relative_to_mount); - - done: - free(memory_hierarchy_mount); - free(mem_cgroup_path_relative_to_mount); - return memory_cgroup_path; - } - - static char* FindCpuCgroupPath(){ - char *cpu_cgroup_path = nullptr; - char *cpu_hierarchy_mount = nullptr; - char *cpu_cgroup_path_relative_to_mount = nullptr; + static char* FindCgroupPath(bool (*is_subsystem)(const char *)){ + char *cgroup_path = nullptr; + char *hierarchy_mount = nullptr; + char *hierarchy_root = nullptr; + char *cgroup_path_relative_to_mount = nullptr; - cpu_hierarchy_mount = FindHierarchyMount(&IsCpuSubsystem); - if (cpu_hierarchy_mount == nullptr) + FindHierarchyMount(is_subsystem, &hierarchy_mount, &hierarchy_root); + if (hierarchy_mount == nullptr || hierarchy_root == nullptr) goto done; - cpu_cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(&IsCpuSubsystem); - if (cpu_cgroup_path_relative_to_mount == nullptr) + cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(is_subsystem); + if (cgroup_path_relative_to_mount == nullptr) goto done; - cpu_cgroup_path = (char*)malloc(strlen(cpu_hierarchy_mount) + strlen(cpu_cgroup_path_relative_to_mount) + 1); - if (cpu_cgroup_path == nullptr) + cgroup_path = (char*)malloc(strlen(hierarchy_mount) + strlen(cgroup_path_relative_to_mount) + 1); + if (cgroup_path == nullptr) goto done; - strcpy(cpu_cgroup_path, cpu_hierarchy_mount); - strcat(cpu_cgroup_path, cpu_cgroup_path_relative_to_mount); + strcpy(cgroup_path, hierarchy_mount); + // For a host cgroup, we need to append the relative path. + // In a docker container, the root and relative path are the same and we don't need to append. + if (strcmp(hierarchy_root, cgroup_path_relative_to_mount) != 0) + strcat(cgroup_path, cgroup_path_relative_to_mount); done: - free(cpu_hierarchy_mount); - free(cpu_cgroup_path_relative_to_mount); - return cpu_cgroup_path; + free(hierarchy_mount); + free(hierarchy_root); + free(cgroup_path_relative_to_mount); + return cgroup_path; } - static char* FindHierarchyMount(bool (*is_subsystem)(const char *)) + static void FindHierarchyMount(bool (*is_subsystem)(const char *), char** pmountpath, char** pmountroot) { char *line = nullptr; size_t lineLen = 0, maxLineLen = 0; char *filesystemType = nullptr; char *options = nullptr; char *mountpath = nullptr; + char *mountroot = nullptr; FILE *mountinfofile = fopen(PROC_MOUNTINFO_FILENAME, "r"); if (mountinfofile == nullptr) @@ -213,16 +193,21 @@ class CGroup mountpath = (char*)malloc(lineLen+1); if (mountpath == nullptr) goto done; + mountroot = (char*)malloc(lineLen+1); + if (mountroot == nullptr) + goto done; sscanfRet = sscanf(line, - "%*s %*s %*s %*s %s ", + "%*s %*s %*s %s %s ", + mountroot, mountpath); - if (sscanfRet != 1) - { - free(mountpath); - mountpath = nullptr; + if (sscanfRet != 2) assert(!"Failed to parse mount info file contents with sscanf."); - } + + // assign the output arguments and clear the locals so we don't free them. + *pmountpath = mountpath; + *pmountroot = mountroot; + mountpath = mountroot = nullptr; goto done; } strTok = strtok_r(nullptr, ",", &context); @@ -230,12 +215,13 @@ class CGroup } } done: + free(mountpath); + free(mountroot); free(filesystemType); free(options); free(line); if (mountinfofile) fclose(mountinfofile); - return mountpath; } static char* FindCGroupPathForSubsystem(bool (*is_subsystem)(const char *)) diff --git a/src/pal/src/misc/cgroup.cpp b/src/pal/src/misc/cgroup.cpp index 84e56ac55d23..ec0a0bd5fb2b 100644 --- a/src/pal/src/misc/cgroup.cpp +++ b/src/pal/src/misc/cgroup.cpp @@ -31,8 +31,8 @@ class CGroup public: CGroup() { - m_memory_cgroup_path = FindMemoryCgroupPath(); - m_cpu_cgroup_path = FindCpuCgroupPath(); + m_memory_cgroup_path = FindCgroupPath(&IsMemorySubsystem); + m_cpu_cgroup_path = FindCgroupPath(&IsCpuSubsystem); } ~CGroup() @@ -105,71 +105,48 @@ class CGroup return strcmp("cpu", strTok) == 0; } - static char* FindMemoryCgroupPath(){ - char *memory_cgroup_path = nullptr; - char *memory_hierarchy_mount = nullptr; - char *mem_cgroup_path_relative_to_mount = nullptr; - size_t len; - - memory_hierarchy_mount = FindHierarchyMount(&IsMemorySubsystem); - if (memory_hierarchy_mount == nullptr) - goto done; - - mem_cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(&IsMemorySubsystem); - if (mem_cgroup_path_relative_to_mount == nullptr) - goto done; - - len = strlen(memory_hierarchy_mount); - len += strlen(mem_cgroup_path_relative_to_mount); - memory_cgroup_path = (char*)PAL_malloc(len+1); - if (memory_cgroup_path == nullptr) - goto done; - - strcpy_s(memory_cgroup_path, len+1, memory_hierarchy_mount); - strcat_s(memory_cgroup_path, len+1, mem_cgroup_path_relative_to_mount); - - done: - PAL_free(memory_hierarchy_mount); - PAL_free(mem_cgroup_path_relative_to_mount); - return memory_cgroup_path; - } - - static char* FindCpuCgroupPath(){ - char *cpu_cgroup_path = nullptr; - char *cpu_hierarchy_mount = nullptr; - char *cpu_cgroup_path_relative_to_mount = nullptr; + static char* FindCgroupPath(bool (*is_subsystem)(const char *)){ + char *cgroup_path = nullptr; + char *hierarchy_mount = nullptr; + char *hierarchy_root = nullptr; + char *cgroup_path_relative_to_mount = nullptr; size_t len; - cpu_hierarchy_mount = FindHierarchyMount(&IsCpuSubsystem); - if (cpu_hierarchy_mount == nullptr) + FindHierarchyMount(is_subsystem, &hierarchy_mount, &hierarchy_root); + if (hierarchy_mount == nullptr || hierarchy_root == nullptr) goto done; - cpu_cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(&IsCpuSubsystem); - if (cpu_cgroup_path_relative_to_mount == nullptr) + cgroup_path_relative_to_mount = FindCGroupPathForSubsystem(is_subsystem); + if (cgroup_path_relative_to_mount == nullptr) goto done; - len = strlen(cpu_hierarchy_mount); - len += strlen(cpu_cgroup_path_relative_to_mount); - cpu_cgroup_path = (char*)PAL_malloc(len+1); - if (cpu_cgroup_path == nullptr) + len = strlen(hierarchy_mount); + len += strlen(cgroup_path_relative_to_mount); + cgroup_path = (char*)PAL_malloc(len+1); + if (cgroup_path == nullptr) goto done; - strcpy_s(cpu_cgroup_path, len+1, cpu_hierarchy_mount); - strcat_s(cpu_cgroup_path, len+1, cpu_cgroup_path_relative_to_mount); + strcpy_s(cgroup_path, len+1, hierarchy_mount); + // For a host cgroup, we need to append the relative path. + // In a docker container, the root and relative path are the same and we don't need to append. + if (strcmp(hierarchy_root, cgroup_path_relative_to_mount) != 0) + strcat_s(cgroup_path, len+1, cgroup_path_relative_to_mount); done: - PAL_free(cpu_hierarchy_mount); - PAL_free(cpu_cgroup_path_relative_to_mount); - return cpu_cgroup_path; + PAL_free(hierarchy_mount); + PAL_free(hierarchy_root); + PAL_free(cgroup_path_relative_to_mount); + return cgroup_path; } - static char* FindHierarchyMount(bool (*is_subsystem)(const char *)) + static void FindHierarchyMount(bool (*is_subsystem)(const char *), char** pmountpath, char** pmountroot) { char *line = nullptr; size_t lineLen = 0, maxLineLen = 0; char *filesystemType = nullptr; char *options = nullptr; char *mountpath = nullptr; + char *mountroot = nullptr; FILE *mountinfofile = fopen(PROC_MOUNTINFO_FILENAME, "r"); if (mountinfofile == nullptr) @@ -213,16 +190,21 @@ class CGroup mountpath = (char*)PAL_malloc(lineLen+1); if (mountpath == nullptr) goto done; + mountroot = (char*)PAL_malloc(lineLen+1); + if (mountroot == nullptr) + goto done; sscanfRet = sscanf_s(line, - "%*s %*s %*s %*s %s ", + "%*s %*s %*s %s %s ", + mountroot, lineLen+1, mountpath, lineLen+1); - if (sscanfRet != 1) - { - PAL_free(mountpath); - mountpath = nullptr; + if (sscanfRet != 2) _ASSERTE(!"Failed to parse mount info file contents with sscanf_s."); - } + + // assign the output arguments and clear the locals so we don't free them. + *pmountpath = mountpath; + *pmountroot = mountroot; + mountpath = mountroot = nullptr; goto done; } strTok = strtok_s(nullptr, ",", &context); @@ -230,12 +212,13 @@ class CGroup } } done: + PAL_free(mountpath); + PAL_free(mountroot); PAL_free(filesystemType); PAL_free(options); free(line); if (mountinfofile) fclose(mountinfofile); - return mountpath; } static char* FindCGroupPathForSubsystem(bool (*is_subsystem)(const char *))