From 16dec13a18c4fae5c7cf6441f75bfa354151f1b4 Mon Sep 17 00:00:00 2001 From: Dhaval Giani Date: Tue, 6 Jan 2009 08:47:57 +0000 Subject: libcgroup: Fix low hanging cleanups Some of the cleanups possible are obvious. 1. Change usage of strcat to strncat 2. Change usage of tge following type char *s = malloc(); strcpy(s, "somestring"); strcat(s, "someotherstring"); to something more easily understandble such as asprintf(&s, "%s%s", somestring, someotherstring); Changes for v2: 1. Fix some memory leaks discovered using valgrind 2. Fix Balbir's comments regarding codingstyle. 3. Move the controllers array memory leak fix into another patch Changes from v1: 1. Correct the error handling of asprintf as pointed out by Dan Smith TODO: 1. Figure out what is the correct value of n for cg_build_path_locked valgrind output, [root@gondor tests]# sh runlibcgrouptest.sh Running first set of testcases ============================== ==5067== Memcheck, a memory error detector. ==5067== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al. ==5067== Using LibVEX rev 1804, a library for dynamic binary translation. ==5067== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP. ==5067== Using valgrind-3.3.0, a dynamic binary instrumentation framework. ==5067== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al. ==5067== For more details, rerun with: -v ==5067== C:DBG: fs_mounted as recieved from script=0 TEST 1:PASS : cgroup_init() Ret Value = 50001 TEST 2:PASS : cgroup_attach_task() Ret Value = 50014 Parameter nullcgroup TEST 3:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 4:PASS : cgroup_create_cgroup() Ret Value = 50014 TEST 5:PASS : cgroup_delete_cgroup() Ret Value = 50014 TEST 6:PASS : cgroup_create_cgroup() Ret Value = 50014 TEST 7:PASS : cgroup_delete_cgroup() Ret Value = 50014 TEST 8:PASS : cgroup_add_controller() Ret Value = 0 TEST 9:PASS : cgroup_add_controller() Ret Value = 0 TEST10:PASS : cgroup_add_controller() Ret Value = 0 ==5067== ==5067== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 18 from 1) ==5067== malloc/free: in use at exit: 0 bytes in 0 blocks. ==5067== malloc/free: 18 allocs, 18 frees, 32,293 bytes allocated. ==5067== For counts of detected errors, rerun with: -v ==5067== All heap blocks were freed -- no leaks are possible. Running second set of testcases ============================== ==5083== Memcheck, a memory error detector. ==5083== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al. ==5083== Using LibVEX rev 1804, a library for dynamic binary translation. ==5083== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP. ==5083== Using valgrind-3.3.0, a dynamic binary instrumentation framework. ==5083== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al. ==5083== For more details, rerun with: -v ==5083== C:DBG: fs_mounted as recieved from script=1 C:DBG: mountpoint1 as recieved from script=/dev/cgroup_controllers-1 sanity check pass. cgroup TEST 1:PASS : cgroup_attach_task() Ret Value = 50014 Parameter nullcgroup TEST 2:PASS : cgroup_init() Ret Value = 0 TEST 3:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s TEST 4:PASS : cgroup_attach_task_pid() Ret Value = 50016 TEST 5:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 6:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST 7:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x40070D8: strncat (mc_replace_strmem.c:214) ==5083== by 0x804BC92: main (libcgrouptest01.c:1271) ==5083== at 0x40070D8: strncat (mc_replace_strmem.c:214) ==5083== by 0x804BC92: main (libcgrouptest01.c:1271) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x8049471: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x8049479: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x4007470: strncmp (mc_replace_strmem.c:314) ==5083== by 0x804949B: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x4007475: strncmp (mc_replace_strmem.c:314) ==5083== by 0x804949B: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x4007497: strncmp (mc_replace_strmem.c:314) ==5083== by 0x804949B: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) ==5083== ==5083== Conditional jump or move depends on uninitialised value(s) ==5083== at 0x40074A0: strncmp (mc_replace_strmem.c:314) ==5083== by 0x804949B: _ZL14group_modifiedPci (libcgrouptest01.c:1076) ==5083== by 0x804C583: main (libcgrouptest01.c:254) TEST 8:FAIL : cgroup_modify_cgroup() Ret Value = 0 Parameter same cgroup TEST 9:PASS : cgroup_new_cgroup() Ret Value = 0 TEST10:PASS : cgroup_modify_cgroup() Ret Value = 0 TEST11:PASS : cgroup_modify_cgroup() Ret Value = 50007 TEST12:PASS : cgroup_new_cgroup() Ret Value = 0 TEST13:PASS : cgroup_modify_cgroup() Ret Value = 0 TEST14:PASS : cgroup_get_cgroup() Ret Value = 50007 Parameter nullcgroup TEST15:PASS : cgroup_get_cgroup() Ret Value = 50002 Parameter not created group TEST16:PASS : cgroup_get_cgroup() Ret Value = 0 TEST 0:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 0:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST17:FAIL : cgroup_get_cgroup() Ret Value = 50017 TEST16:PASS : cgroup_delete_cgroup() Ret Value = 0 Group deleted from filesystem TEST17:PASS : cgroup_create_cgroup() Ret Value = 50007 TEST18:PASS : cgroup_delete_cgroup() Ret Value = 50007 TEST19:PASS : cgroup_compare_cgroup() Ret Value = 50011 Parameter nullcgroup TEST20:PASS : cgroup_compare_cgroup() Ret Value = 0 TEST21:PASS : cgroup_compare_cgroup() Ret Value = 50017 ==5083== ==5083== ERROR SUMMARY: 9 errors from 7 contexts (suppressed: 18 from 1) ==5083== malloc/free: in use at exit: 0 bytes in 0 blocks. ==5083== malloc/free: 279 allocs, 279 frees, 237,968 bytes allocated. ==5083== For counts of detected errors, rerun with: -v ==5083== All heap blocks were freed -- no leaks are possible. Cleanup done Running third set of testcases ============================== ==5134== Memcheck, a memory error detector. ==5134== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al. ==5134== Using LibVEX rev 1804, a library for dynamic binary translation. ==5134== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP. ==5134== Using valgrind-3.3.0, a dynamic binary instrumentation framework. ==5134== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al. ==5134== For more details, rerun with: -v ==5134== C:DBG: fs_mounted as recieved from script=2 C:DBG: mountpoint1 as recieved from script=/dev/cgroup_controllers-1 C:DBG: mountpoint2 as recieved from script=/dev/cgroup_controllers-2 sanity check pass. cgroup TEST 1:PASS : cgroup_init() Ret Value = 0 TEST 2:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s TEST 3:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 4:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST 5:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 6:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST 5:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 6:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST 7:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST 8:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s TEST 9:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s TEST10:PASS : cgroup_new_cgroup() Ret Value = 0 TEST11:PASS : cgroup_attach_task() Ret Value = 50002 Parameter not created group TEST12:PASS : cgroup_new_cgroup() Ret Value = 0 TEST13:PASS : cgroup_modify_cgroup() Ret Value = 0 TEST14:PASS : cgroup_new_cgroup() Ret Value = 0 TEST15:PASS : cgroup_modify_cgroup() Ret Value = 0 TEST16:PASS : cgroup_delete_cgroup() Ret Value = 0 Group deleted from filesystem TEST17:PASS : cgroup_delete_cgroup() Ret Value = 0 Group deleted from filesystem TEST18:PASS : cgroup_new_cgroup() Ret Value = 0 TEST19:PASS : cgroup_create_cgroup() Ret Value = 0 group found under both controllers TEST20:PASS : cgroup_attach_task() Ret Value = 0 Task found in group/s TEST21:PASS : cgroup_new_cgroup() Ret Value = 0 TEST22:PASS : cgroup_modify_cgroup() Ret Value = 0 group modified under both controllers TEST23:PASS : cgroup_delete_cgroup() Ret Value = 0 Group deleted from filesystem TEST 0:PASS : cgroup_new_cgroup() Ret Value = 0 TEST 0:PASS : cgroup_create_cgroup() Ret Value = 0 group found in filesystem TEST24:FAIL : cgroup_get_cgroup() Ret Value = 50018 ==5134== ==5134== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 18 from 1) ==5134== malloc/free: in use at exit: 66,044 bytes in 15 blocks. ==5134== malloc/free: 279 allocs, 264 frees, 232,094 bytes allocated. ==5134== For counts of detected errors, rerun with: -v ==5134== searching for pointers to 15 not-freed blocks. ==5134== checked 6,381,388 bytes. ==5134== ==5134== 66,044 (18,064 direct, 47,980 indirect) bytes in 4 blocks are definitely lost in loss record 1 of 3 ==5134== at 0x4004BA2: calloc (vg_replace_malloc.c:397) ==5134== by 0x4030DAA: cgroup_new_cgroup (wrapper.c:28) ==5134== by 0x8049656: new_cgroup(char*, char*, char*, int, int) (libcgrouptest01.c:1132) ==5134== by 0x8049917: create_new_cgroup_ds(int, char const*, int, int) (libcgrouptest01.c:744) ==5134== by 0x804C190: main (libcgrouptest01.c:485) ==5134== ==5134== ==5134== 20,980 bytes in 5 blocks are indirectly lost in loss record 2 of 3 ==5134== at 0x4004BA2: calloc (vg_replace_malloc.c:397) ==5134== by 0x4030C3C: cgroup_add_value_string (wrapper.c:122) ==5134== by 0x8048FDB: _ZL17add_control_valueP17cgroup_controllerPcS1_i (libcgrouptest01.c:1113) ==5134== by 0x80496C9: new_cgroup(char*, char*, char*, int, int) (libcgrouptest01.c:1147) ==5134== by 0x8049917: create_new_cgroup_ds(int, char const*, int, int) (libcgrouptest01.c:744) ==5134== by 0x804C190: main (libcgrouptest01.c:485) ==5134== ==5134== ==5134== 27,000 bytes in 6 blocks are indirectly lost in loss record 3 of 3 ==5134== at 0x4004BA2: calloc (vg_replace_malloc.c:397) ==5134== by 0x4030F5B: cgroup_add_controller (wrapper.c:62) ==5134== by 0x80496AE: new_cgroup(char*, char*, char*, int, int) (libcgrouptest01.c:1144) ==5134== by 0x8049917: create_new_cgroup_ds(int, char const*, int, int) (libcgrouptest01.c:744) ==5134== by 0x804C190: main (libcgrouptest01.c:485) ==5134== ==5134== LEAK SUMMARY: ==5134== definitely lost: 18,064 bytes in 4 blocks. ==5134== indirectly lost: 47,980 bytes in 11 blocks. ==5134== possibly lost: 0 bytes in 0 blocks. ==5134== still reachable: 0 bytes in 0 blocks. ==5134== suppressed: 0 bytes in 0 blocks. Cleanup done [root@gondor tests]# Signed-off-by: Dhaval Giani Acked-by: Balbir Singh git-svn-id: https://libcg.svn.sourceforge.net/svnroot/libcg/trunk@302 4f4bb910-9a46-0410-90c8-c897d4f1cd53 --- api.c | 107 ++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 69 insertions(+), 38 deletions(-) diff --git a/api.c b/api.c index 2f2936a..eb94303 100644 --- a/api.c +++ b/api.c @@ -580,8 +580,7 @@ int cgroup_init() &hierarchy, &num_cgroups, &enabled); if (err < 0) break; - controllers[i] = (char *)malloc(strlen(subsys_name) + 1); - strcpy(controllers[i], subsys_name); + controllers[i] = strdup(subsys_name); i++; } controllers[i] = NULL; @@ -647,10 +646,13 @@ unlock_exit: if (proc_mount) fclose(proc_mount); - for (i = 0; controllers[i]; i++) + for (i = 0; controllers[i]; i++) { free(controllers[i]); + controllers[i] = NULL; + } pthread_rwlock_unlock(&cg_mount_table_lock); + return ret; } @@ -704,12 +706,16 @@ static char *cg_build_path_locked(char *name, char *path, char *type) { int i; for (i = 0; cg_mount_table[i].name[0] != '\0'; i++) { + /* + * XX: Change to snprintf once you figure what n should be + */ if (strcmp(cg_mount_table[i].name, type) == 0) { - strcpy(path, cg_mount_table[i].path); - strcat(path, "/"); + sprintf(path, "%s/", cg_mount_table[i].path); if (name) { - strcat(path, name); - strcat(path, "/"); + char *tmp; + tmp = strdup(path); + sprintf(path, "%s%s/", tmp, name); + free(tmp); } return path; } @@ -752,7 +758,7 @@ int cgroup_attach_task_pid(struct cgroup *cgroup, pid_t tid) if (!cg_build_path_locked(NULL, path, cg_mount_table[i].name)) continue; - strcat(path, "/tasks"); + strncat(path, "/tasks", sizeof(path) - strlen(path)); tasks = fopen(path, "w"); if (!tasks) { @@ -798,7 +804,7 @@ int cgroup_attach_task_pid(struct cgroup *cgroup, pid_t tid) cgroup->controller[i]->name)) continue; - strcat(path, "/tasks"); + strncat(path, "/tasks", sizeof(path) - strlen(path)); tasks = fopen(path, "w"); if (!tasks) { @@ -966,7 +972,7 @@ static int cg_set_control_value(char *path, char *val) while (*(path+len) != '/') len--; *(path+len+1) = '\0'; - strcat(path, "tasks"); + strncat(path, "tasks", sizeof(path) - strlen(path)); control_file = fopen(path, "r"); if (!control_file) { if (errno == ENOENT) @@ -996,9 +1002,10 @@ static int cg_set_control_value(char *path, char *val) int cgroup_modify_cgroup(struct cgroup *cgroup) { - char path[FILENAME_MAX], base[FILENAME_MAX]; + char *path, base[FILENAME_MAX]; int i; int error; + int ret; if (!cgroup_initialized) return ECGROUPNOTINITIALIZED; @@ -1014,24 +1021,32 @@ int cgroup_modify_cgroup(struct cgroup *cgroup) } } - strcpy(path, base); - for (i = 0; i < cgroup->index; i++, strcpy(path, base)) { + for (i = 0; i < cgroup->index; i++) { int j; if (!cg_build_path(cgroup->name, base, cgroup->controller[i]->name)) continue; - strcpy(path, base); - for (j = 0; j < cgroup->controller[i]->index; - j++, strcpy(path, base)) { - strcat(path, cgroup->controller[i]->values[j]->name); + for (j = 0; j < cgroup->controller[i]->index; j++) { + ret = asprintf(&path, "%s%s", base, + cgroup->controller[i]->values[j]->name); + if (ret < 0) { + error = ECGOTHER; + goto err; + } error = cg_set_control_value(path, cgroup->controller[i]->values[j]->value); + free(path); + path = NULL; if (error) goto err; } } + if (path) + free(path); return 0; err: + if (path) + free(path); return error; } @@ -1118,11 +1133,13 @@ err: */ int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) { - char *fts_path[2], base[FILENAME_MAX]; + char *fts_path[2]; + char *base = NULL; char *path = NULL; int i, j, k; int error = 0; int retval = 0; + int ret; if (!cgroup_initialized) return ECGROUPNOTINITIALIZED; @@ -1147,8 +1164,6 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) * data structure. If not, we fail. */ for (k = 0; k < cgroup->index; k++) { - path[0] = '\0'; - if (!cg_build_path(cgroup->name, path, cgroup->controller[k]->name)) continue; @@ -1157,7 +1172,12 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) if (error) goto err; - strcpy(base, path); + base = strdup(path); + + if (!base) { + error = ECGOTHER; + goto err; + } if (!ignore_ownership) error = cg_chown_recursive(fts_path, @@ -1166,9 +1186,14 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) if (error) goto err; - for (j = 0; j < cgroup->controller[k]->index; j++, - strcpy(path, base)) { - strcat(path, cgroup->controller[k]->values[j]->name); + for (j = 0; j < cgroup->controller[k]->index; j++) { + free(path); + ret = asprintf(&path, "%s%s", base, + cgroup->controller[k]->values[j]->name); + if (ret < 0) { + error = ECGOTHER; + goto err; + } error = cg_set_control_value(path, cgroup->controller[k]->values[j]->value); /* @@ -1187,19 +1212,28 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) } if (!ignore_ownership) { - strcpy(path, base); - strcat(path, "/tasks"); + free(path); + ret = asprintf(&path, "%s/tasks", base); + if (ret < 0) { + error = ECGOTHER; + goto err; + } error = chown(path, cgroup->tasks_uid, cgroup->tasks_gid); if (error) { - error = ECGFAIL; + error = ECGOTHER; goto err; } } + free(base); + base = NULL; } err: - free(path); + if (path) + free(path); + if (base) + free(base); if (retval && !error) error = retval; return error; @@ -1335,7 +1369,7 @@ int cgroup_delete_cgroup(struct cgroup *cgroup, int ignore_migration) if (!cg_build_path(cgroup->name, path, cgroup->controller[i]->name)) continue; - strcat(path, "../tasks"); + strncat(path, "../tasks", sizeof(path) - strlen(path)); base_tasks = fopen(path, "w"); if (!base_tasks) @@ -1347,7 +1381,7 @@ int cgroup_delete_cgroup(struct cgroup *cgroup, int ignore_migration) continue; } - strcat(path, "tasks"); + strncat(path, "tasks", sizeof(path) - strlen(path)); delete_tasks = fopen(path, "r"); if (!delete_tasks) { @@ -1401,7 +1435,7 @@ static int cg_rd_ctrl_file(char *subsys, char *cgroup, char *file, char **value) if (!cg_build_path_locked(cgroup, path, subsys)) return ECGFAIL; - strcat(path, file); + strncat(path, file, sizeof(path) - strlen(path)); ctrl_file = fopen(path, "r"); if (!ctrl_file) return ECGROUPVALUENOTEXIST; @@ -1454,7 +1488,7 @@ static int cgroup_fill_cgc(struct dirent *ctrl_dir, struct cgroup *cgroup, */ cg_build_path_locked(cgroup->name, path, cg_mount_table[index].name); - strcat(path, d_name); + strncat(path, d_name, sizeof(path) - strlen(path)); error = stat(path, &stat_buffer); @@ -1513,6 +1547,7 @@ int cgroup_get_cgroup(struct cgroup *cgroup) struct dirent *ctrl_dir = NULL; char *control_path = NULL; int error; + int ret; if (!cgroup_initialized) { /* ECGROUPNOTINITIALIZED */ @@ -1558,17 +1593,13 @@ int cgroup_get_cgroup(struct cgroup *cgroup) * Get the uid and gid information */ - control_path = malloc(strlen(path) + strlen("tasks") + 1); + ret = asprintf(&control_path, "%s/tasks", path); - if (!control_path) { + if (ret < 0) { error = ECGOTHER; goto unlock_error; } - strcpy(control_path, path); - - strcat(control_path, "tasks"); - if (stat(control_path, &stat_buffer)) { free(control_path); error = ECGOTHER; -- cgit