Skip to content

Commit b4b812e

Browse files
api.c: fix cgroup_attach_task_pid() behavior
Originally cgroup_attach_task_pid(cgroup, tid), allowed passing of struct cgroup *cgroup or NULL as a valid argument. In the case of cgroup being passed, the tid was attached to the cgroup by writing into the 'tasks' file of the cgroup. It gets interesting, when the NULL is passed, the idea is to migrate the task to the cgroup_root of the hierarchy of the task is currently running in. Consider a simple legacy cgroup controller hirerachy: cpu,cpuacct memory cpuset / \ / / C1 C2 M1 U1 | | / \ t1 t1 UC1 UC2 | t1 passing NULL: cpu,cpuacct memory cpuset / \ | / | / | C1 C2 t1 M1 t1 U1 t1 / \ UC1 UC2 Depending upon the cgroup setup, legacy (only cgroup v1), unified (cgroup v2) or hybrid (controllers mounted in the cgroup v1 and some of the functionalities available from cgroup v2). This behavior got changed in the due course and will cause a segfault, when passed with NULL as the cgroup. Reproducer: int main(int argc, char *argv[]) { pid_t pid; int ret; ret = cgroup_init(); if (ret) { printf("cgroup_init failed with %s\n", cgroup_strerror(ret)); exit(1); } pid = atoi(argv[1]); ret = cgroup_attach_task_pid(NULL, pid); if (ret) printf("attaching %d to cgroup %s failed\n", cgroup_strerror(ret)); return ret; } Re-introduce the original behavior, that considers all the three possible cgroup modes and performances of the migration based on the modes. fscanf() is in-efficient in parsing /proc/<pid>/cgroup fields with empty values, this is handled by a helper function proc_pid_cgroup_parser(). It was discovered by the Coverity tool as Dereference after null check: CID 258278 (#1 of 1): Dereference after null check (FORWARD_NULL). var_deref_op: Dereferencing null pointer cgroup. Signed-off-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
1 parent 96dce40 commit b4b812e

File tree

1 file changed

+154
-26
lines changed

1 file changed

+154
-26
lines changed

src/api.c

Lines changed: 154 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2142,11 +2142,101 @@ static int cgroup_build_tid_path(const char * const ctrl_name, char *path)
21422142
return ret;
21432143
}
21442144

2145-
static int cgroup_attach_task_tid(struct cgroup *cgrp, pid_t tid, bool move_tids)
2145+
/**
2146+
* proc_pid_cgroup_parser() - Reads a line from /proc/<pid>/cgroup and parses it
2147+
* @fp The /proc/<pid>/cgroup, file handle
2148+
* @num Hierarchy number is filled from parsed line
2149+
* @controllers Controllers names are filled from the parsed line
2150+
* @cgrp_path Cgroup path is filled from the parsed line
2151+
*
2152+
* this function read the next line from the file handle and parses it.
2153+
* On success, returns 0 and fills the values into the function arguments passed and
2154+
* failure returns an error and the function arguments might contain stale values.
2155+
*/
2156+
static int proc_pid_cgroup_parser(FILE **fp, int *num, char *controllers, char *cgrp_path)
2157+
{
2158+
/*
2159+
* /proc/<pid>/cgroup:
2160+
* 7:memory:/user.slice/user-1000.slice/session-3.scope
2161+
* <int>11:CONTROL_NAMELEN_MAX:FILENAME_MAX
2162+
*/
2163+
#define LINE_LEN_MAX (11 + CONTROL_NAMELEN_MAX + FILENAME_MAX)
2164+
char *tmp, *line, *str, *start;
2165+
int len, ret = ECGOTHER;
2166+
2167+
if (!*fp)
2168+
return ECGINVAL;
2169+
2170+
start = line = malloc(LINE_LEN_MAX);
2171+
if (!line) {
2172+
last_errno = errno;
2173+
goto fail;
2174+
}
2175+
2176+
if (fgets(line, (LINE_LEN_MAX), *fp) == NULL) {
2177+
if (feof(*fp))
2178+
ret = ECGEOF;
2179+
goto fail;
2180+
}
2181+
2182+
/*
2183+
* /proc/<pid>/cgroup, has two formats:
2184+
* 7:memory:/user.slice/user-1000.slice/session-3.scope
2185+
* 0::/user.slice/user-1000.slice/session-3.scope
2186+
*/
2187+
str = strstr(line, ":");
2188+
if (str == NULL)
2189+
goto fail;
2190+
2191+
if (num) {
2192+
len = str - line;
2193+
tmp = malloc(11); /* range of int is 10 digits */
2194+
if (!tmp) {
2195+
last_errno = errno;
2196+
goto fail;
2197+
}
2198+
strncpy(tmp, line, len);
2199+
tmp[len] = '\0';
2200+
2201+
*num = atoi(tmp);
2202+
free(tmp);
2203+
}
2204+
2205+
line = str + 1;
2206+
str = strstr(line, ":");
2207+
if (str == NULL)
2208+
goto fail;
2209+
2210+
if (controllers) {
2211+
len = str - line;
2212+
strncpy(controllers, line, len);
2213+
controllers[len] = '\0';
2214+
}
2215+
2216+
line = str + 1;
2217+
if (cgrp_path) {
2218+
len = strlen(line);
2219+
strncpy(cgrp_path, line, len - 1);
2220+
cgrp_path[len - 1] = '\0';
2221+
}
2222+
2223+
ret = 0;
2224+
2225+
fail:
2226+
if (start)
2227+
free(start);
2228+
2229+
return ret;
2230+
}
2231+
2232+
static int cgroup_attach_task_tid(struct cgroup *cgroup, pid_t tid, bool move_tids)
21462233
{
2234+
char controllers[CG_CONTROLLER_MAX];
21472235
char path[FILENAME_MAX] = {0};
2236+
char cgrp_path[FILENAME_MAX];
21482237
char *controller_name = NULL;
21492238
int empty_cgrp = 0;
2239+
FILE *pid_cgrp_fd;
21502240
int i, ret = 0;
21512241

21522242
if (!cgroup_initialized) {
@@ -2155,53 +2245,91 @@ static int cgroup_attach_task_tid(struct cgroup *cgrp, pid_t tid, bool move_tids
21552245
}
21562246

21572247
/* if the cgroup is NULL, attach the task to the root cgroup. */
2158-
if (!cgrp) {
2248+
if (!cgroup) {
2249+
snprintf(path, FILENAME_MAX, "/proc/%d/cgroup", tid);
2250+
pid_cgrp_fd = fopen(path, "re");
2251+
if (!pid_cgrp_fd) {
2252+
cgroup_warn("failed to open /proc/%d/cgroup\n", tid);
2253+
return ECGROUPNOTALLOWED;
2254+
}
2255+
21592256
pthread_rwlock_rdlock(&cg_mount_table_lock);
2160-
for (i = 0; i < CG_CONTROLLER_MAX && cg_mount_table[i].name[0] != '\0'; i++) {
2161-
ret = cgroup_build_tasks_procs_path(path, sizeof(path), NULL,
2162-
cg_mount_table[i].name);
2257+
do {
2258+
ret = proc_pid_cgroup_parser(&pid_cgrp_fd, NULL, controllers, cgrp_path);
21632259
if (ret)
2164-
return ret;
2260+
goto cgrp_root_migration_done;
21652261

2166-
if (move_tids) {
2167-
ret = cgroup_build_tid_path(controller_name, path);
2262+
/*
2263+
* 0:memory:/ means that tid is not part of this hierarchy,
2264+
* so skip migrating task to cgroup_root.
2265+
*/
2266+
if (strlen(cgrp_path) == 1)
2267+
continue;
2268+
2269+
for (i = 0;
2270+
i < CG_CONTROLLER_MAX && cg_mount_table[i].name[0] != '\0';
2271+
i++) {
2272+
2273+
if (!cg_build_path_locked(NULL, path, cg_mount_table[i].name))
2274+
continue;
2275+
2276+
/*
2277+
* legacy mode:
2278+
* 7:memory:/user.slice/user-1000.slice/session-3.scope
2279+
* concatenate /tasks to the patch
2280+
* unified/hybrid:
2281+
* 0::/user.slice/user-1000.slice/session-3.scope
2282+
* concatenate /cgroup.procs to the path
2283+
*/
2284+
if (strncmp(controllers, cg_mount_table[i].name,
2285+
CONTROL_NAMELEN_MAX) == 0)
2286+
strncat(path, "/tasks", FILENAME_MAX - strlen(path));
2287+
else if (strlen(controllers) == 0)
2288+
strncat(path, "/cgroup.procs", FILENAME_MAX - strlen(path));
2289+
else
2290+
continue;
2291+
2292+
cgroup_dbg("attaching %d to %s\n", tid, path);
2293+
ret = __cgroup_attach_task_pid(path, tid);
21682294
if (ret)
2169-
return ret;
2295+
goto cgrp_root_migration_done;
21702296
}
2297+
} while (ret == 0);
2298+
2299+
cgrp_root_migration_done:
2300+
if (ret == ECGEOF)
2301+
ret = 0;
2302+
2303+
if (pid_cgrp_fd)
2304+
fclose(pid_cgrp_fd);
21712305

2172-
ret = __cgroup_attach_task_pid(path, tid);
2173-
if (ret) {
2174-
pthread_rwlock_unlock(&cg_mount_table_lock);
2175-
return ret;
2176-
}
2177-
}
21782306
pthread_rwlock_unlock(&cg_mount_table_lock);
21792307
} else {
2180-
for (i = 0; i < cgrp->index; i++) {
2181-
if (!cgroup_test_subsys_mounted(cgrp->controller[i]->name)) {
2308+
for (i = 0; i < cgroup->index; i++) {
2309+
if (!cgroup_test_subsys_mounted(cgroup->controller[i]->name)) {
21822310
cgroup_warn("subsystem %s is not mounted\n",
2183-
cgrp->controller[i]->name);
2311+
cgroup->controller[i]->name);
21842312
return ECGROUPSUBSYSNOTMOUNTED;
21852313
}
21862314
}
21872315

2188-
if (cgrp->index == 0)
2316+
if (cgroup->index == 0)
21892317
/* Valid empty cgroup v2 with no controllers added. */
21902318
empty_cgrp = 1;
21912319

21922320
for (i = 0, controller_name = NULL;
2193-
empty_cgrp > 0 || i < cgrp->index;
2194-
i++, empty_cgrp--) {
2321+
empty_cgrp > 0 || i < cgroup->index;
2322+
i++, empty_cgrp--) {
21952323

2196-
if (cgrp->controller[i])
2197-
controller_name = cgrp->controller[i]->name;
2324+
if (cgroup->controller[i])
2325+
controller_name = cgroup->controller[i]->name;
21982326

2199-
ret = cgroupv2_controller_enabled(cgrp->name, controller_name);
2327+
ret = cgroupv2_controller_enabled(cgroup->name, controller_name);
22002328
if (ret)
22012329
return ret;
22022330

2203-
ret = cgroup_build_tasks_procs_path(path, sizeof(path), cgrp->name,
2204-
controller_name);
2331+
ret = cgroup_build_tasks_procs_path(path, sizeof(path), cgroup->name,
2332+
controller_name);
22052333
if (ret)
22062334
return ret;
22072335

0 commit comments

Comments
 (0)