Fix pal cgroup v2 implementation (#44990)

* Fix pal cgroup v2 implementation Fixes two issues in src/pal/src/misc/cgroup.cpp: * No subsystem match must be performed for cgroup v2. * Incorrect arguments for sscanf_s when reading cgroup path. The src/gc/unix/cgroup.cpp implementation doesn't have these issues. * Rename is_subsystem_match to isSubsystemMatch
dotnet · Nov 20, 2020 · 70c51bb · 70c51bb
1 parent e2312e1
commit 70c51bb
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 36 deletions.
diff --git a/src/coreclr/src/gc/unix/cgroup.cpp b/src/coreclr/src/gc/unix/cgroup.cpp
@@ -66,8 +66,8 @@ class CGroup
     static void Initialize()
     {
         s_cgroup_version = FindCGroupVersion();
-        s_memory_cgroup_path = FindCGroupPath(&IsCGroup1MemorySubsystem);
-        s_cpu_cgroup_path = FindCGroupPath(&IsCGroup1CpuSubsystem);
+        s_memory_cgroup_path = FindCGroupPath(s_cgroup_version == 1 ? &IsCGroup1MemorySubsystem : nullptr);
+        s_cpu_cgroup_path = FindCGroupPath(s_cgroup_version == 1 ? &IsCGroup1CpuSubsystem : nullptr);
     }
 
     static void Cleanup()
@@ -257,12 +257,19 @@ class CGroup
 
             if (strncmp(filesystemType, "cgroup", 6) == 0)
             {
-                char* context = nullptr;
-                char* strTok = strtok_r(options, ",", &context);
-                while (strTok != nullptr)
+                bool isSubsystemMatch = is_subsystem == nullptr;
+                if (!isSubsystemMatch)
                 {
-                    if ((s_cgroup_version == 2) || ((s_cgroup_version == 1) && is_subsystem(strTok)))
+                    char* context = nullptr;
+                    char* strTok = strtok_r(options, ",", &context);
+                    while (!isSubsystemMatch && strTok != nullptr)
                     {
+                        isSubsystemMatch = is_subsystem(strTok);
+                        strTok = strtok_r(nullptr, ",", &context);
+                    }
+                }
+                if (isSubsystemMatch)
+                {
                         mountpath = (char*)malloc(lineLen+1);
                         if (mountpath == nullptr)
                             goto done;
@@ -281,9 +288,6 @@ class CGroup
                         *pmountpath = mountpath;
                         *pmountroot = mountroot;
                         mountpath = mountroot = nullptr;
-                        goto done;
-                    }
-                    strTok = strtok_r(nullptr, ",", &context);
                 }
             }
         }

diff --git a/src/coreclr/src/pal/src/misc/cgroup.cpp b/src/coreclr/src/pal/src/misc/cgroup.cpp
@@ -53,8 +53,8 @@ class CGroup
     static void Initialize()
     {
         s_cgroup_version = FindCGroupVersion();
-        s_memory_cgroup_path = FindCGroupPath(&IsCGroup1MemorySubsystem);
-        s_cpu_cgroup_path = FindCGroupPath(&IsCGroup1CpuSubsystem);
+        s_memory_cgroup_path = FindCGroupPath(s_cgroup_version == 1 ? &IsCGroup1MemorySubsystem : nullptr);
+        s_cpu_cgroup_path = FindCGroupPath(s_cgroup_version == 1 ? &IsCGroup1CpuSubsystem : nullptr);
     }
 
     static void Cleanup()
@@ -245,33 +245,37 @@ class CGroup
 
             if (strncmp(filesystemType, "cgroup", 6) == 0)
             {
-                char* context = nullptr;
-                char* strTok = strtok_s(options, ",", &context);
-                while (strTok != nullptr)
+                bool isSubsystemMatch = is_subsystem == nullptr;
+                if (!isSubsystemMatch)
                 {
-                    if (is_subsystem(strTok))
+                    char* context = nullptr;
+                    char* strTok = strtok_s(options, ",", &context);
+                    while (!isSubsystemMatch && strTok != nullptr)
                     {
-                        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 ",
-                                             mountroot, lineLen+1,
-                                             mountpath, lineLen+1);
-                        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;
+                        isSubsystemMatch = is_subsystem(strTok);
+                        strTok = strtok_s(nullptr, ",", &context);
                     }
-                    strTok = strtok_s(nullptr, ",", &context);
+                }
+                if (isSubsystemMatch)
+                {
+                    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 ",
+                                        mountroot, lineLen+1,
+                                        mountpath, lineLen+1);
+                    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;
                 }
             }
         }
@@ -343,7 +347,7 @@ class CGroup
                 // See https://www.kernel.org/doc/Documentation/cgroup-v2.txt
                 // Look for a "0::/some/path"
                 int sscanfRet = sscanf_s(line,
-                                         "0::%s", lineLen+1,
+                                         "0::%s",
                                          cgroup_path, lineLen+1);
                 if (sscanfRet == 1)
                 {