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 <dhaval@linux.vnet.ibm.com>
Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>


git-svn-id: https://libcg.svn.sourceforge.net/svnroot/libcg/trunk@302 4f4bb910-9a46-0410-90c8-c897d4f1cd53

1 files 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;