At a number of places I noticed a following combination of functions build_path; strcat;
Realizing that the way build_path is designed is not very conducive to such a scenario, and also requires memory to have been pre-allocated which caused a whole lot of other issues, such as using statically sized arrays which could overflow, not giving us the opportunity to reallocate, it looks like to have this sort of a function is a better option. We can now have dynamically allocated strings which will help us move from static arrays to dynamic arrays. As of now, only build tested Changes from v1 1. Used vasprintf as pointed out by Dan Smith. Minimal testing shows the usage is correct, but would require closer review 2. Corrected some other bugs which were pointed out in testing 3. Minimally tested using the test cases Sudhir has submitted. Signed-off-by: Dhaval Giani <[email protected]> --- api.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 125 insertions(+), 29 deletions(-) Index: trunk/api.c =================================================================== --- trunk.orig/api.c 2009-01-02 14:21:37.000000000 +0530 +++ trunk/api.c 2009-01-02 14:23:34.000000000 +0530 @@ -41,6 +41,7 @@ #include <pwd.h> #include <libgen.h> #include <assert.h> +#include <stdarg.h> #ifndef PACKAGE_VERSION #define PACKAGE_VERSION 0.01 @@ -687,7 +688,68 @@ static inline pid_t cg_gettid() return syscall(__NR_gettid); } +/* + * Always take the mount table lock before calling in here. + */ +static char *cg_build_path_var(char *name, char *type, const char *fmt, ...) +{ + char *path = NULL, *tmp = NULL, *tmp2 = NULL; + int i, size, ret; + bool found = false; + va_list args; + + for (i = 0; cg_mount_table[i].name[0] != '\0'; i++) { + if (strcmp(cg_mount_table[i].name, type) == 0) { + found = true; + if (name) + asprintf(&tmp, "%s/%s", cg_mount_table[i].path, + name); + else + asprintf(&tmp, "%s", cg_mount_table[i].path); + if (!name) + return NULL; + break; + } + } + /* + * If !found, then tmp never got allocated and so we don't need + * to free it + */ + if (!found) + goto err; + + if (!fmt) + goto err; + + size = 1024; + tmp2 = calloc(1, 1024); + + if (!tmp2) + goto err; + + va_start(args, fmt); + ret = vasprintf(&tmp2, fmt, args); + va_end(args); + + if (ret < 0) + goto err; + + ret = asprintf(&path, "%s/%s", tmp, tmp2); + + if (ret < 0) + path = NULL; +err: + if (tmp) + free(tmp); + if (tmp2) + free(tmp2); + + /* + * Returning path is fine, if asprintf failed, it will return NULL + */ + return path; +} /* Call with cg_mount_table_lock taken */ static char *cg_build_path_locked(char *name, char *path, char *type) { @@ -1325,7 +1387,7 @@ int cgroup_delete_cgroup(struct cgroup * { FILE *delete_tasks, *base_tasks = NULL; int tids; - char path[FILENAME_MAX]; + char *path = NULL; int error = ECGROUPNOTALLOWED; int i, ret; @@ -1341,24 +1403,31 @@ int cgroup_delete_cgroup(struct cgroup * } for (i = 0; i < cgroup->index; i++) { - if (!cg_build_path(cgroup->name, path, - cgroup->controller[i]->name)) + pthread_rwlock_rdlock(&cg_mount_table_lock); + path = cg_build_path_var(cgroup->name, + cgroup->controller[i]->name, "../tasks"); + pthread_rwlock_unlock(&cg_mount_table_lock); + + if (!path) continue; - strncat(path, "../tasks", sizeof(path) - strlen(path)); base_tasks = fopen(path, "w"); + free(path); + path = NULL; if (!base_tasks) goto open_err; - if (!cg_build_path(cgroup->name, path, - cgroup->controller[i]->name)) { - fclose(base_tasks); - continue; - } + pthread_rwlock_rdlock(&cg_mount_table_lock); + path = cg_build_path_var(cgroup->name, + cgroup->controller[i]->name, "tasks"); + pthread_rwlock_unlock(&cg_mount_table_lock); - strncat(path, "tasks", sizeof(path) - strlen(path)); + if (!path) + continue; delete_tasks = fopen(path, "r"); + free(path); + path = NULL; if (!delete_tasks) { fclose(base_tasks); goto open_err; @@ -1374,6 +1443,16 @@ int cgroup_delete_cgroup(struct cgroup * fclose(delete_tasks); fclose(base_tasks); + if (path) + free(path); + + path = calloc(FILENAME_MAX, 1); + + if (path) { + error = ECGOTHER; + goto open_err; + } + if (!cg_build_path(cgroup->name, path, cgroup->controller[i]->name)) continue; @@ -1403,15 +1482,18 @@ open_err: */ static int cg_rd_ctrl_file(char *subsys, char *cgroup, char *file, char **value) { - char path[FILENAME_MAX]; + char *path; FILE *ctrl_file; int ret; - if (!cg_build_path_locked(cgroup, path, subsys)) + path = cg_build_path_var(cgroup, subsys, "%s", file); + + if (!path) return ECGFAIL; - strncat(path, file, sizeof(path) - strlen(path)); ctrl_file = fopen(path, "r"); + + free(path); if (!ctrl_file) return ECGROUPVALUENOTEXIST; @@ -1444,7 +1526,7 @@ static int cgroup_fill_cgc(struct dirent char *ctrl_file; char *ctrl_value = NULL; char *d_name; - char path[FILENAME_MAX+1]; + char *path = NULL; char *buffer; int error = 0; struct stat stat_buffer; @@ -1462,8 +1544,14 @@ static int cgroup_fill_cgc(struct dirent * some sort of a flag, but this is fine for now. */ - cg_build_path_locked(cgroup->name, path, cg_mount_table[index].name); - strncat(path, d_name, sizeof(path) - strlen(path)); + pthread_rwlock_rdlock(&cg_mount_table_lock); + path = cg_build_path_var(cgroup->name, cg_mount_table[index].name, + "%s", d_name); + pthread_rwlock_unlock(&cg_mount_table_lock); + if (!path) { + error = ECGOTHER; + goto fill_error; + } error = stat(path, &stat_buffer); @@ -1502,6 +1590,8 @@ static int cgroup_fill_cgc(struct dirent } } fill_error: + if (path) + free(path); if (ctrl_value) free(ctrl_value); free(d_name); @@ -1517,7 +1607,7 @@ fill_error: int cgroup_get_cgroup(struct cgroup *cgroup) { int i; - char path[FILENAME_MAX]; + char *path = NULL; DIR *dir; struct dirent *ctrl_dir; char *control_path; @@ -1545,24 +1635,27 @@ int cgroup_get_cgroup(struct cgroup *cgr struct stat stat_buffer; int path_len; - if (!cg_build_path_locked(NULL, path, - cg_mount_table[i].name)) - continue; + pthread_rwlock_rdlock(&cg_mount_table_lock); + path = cg_build_path_var(NULL, cg_mount_table[i].name, "%s", + cgroup->name); + pthread_rwlock_unlock(&cg_mount_table_lock); - path_len = strlen(path); - strncat(path, cgroup->name, FILENAME_MAX - path_len - 1); + if (!path) { + error = ECGOTHER; + goto unlock_error; + } if (access(path, F_OK)) continue; - if (!cg_build_path_locked(cgroup->name, path, - cg_mount_table[i].name)) { - /* - * This fails when the cgroup does not exist - * for that controller. - */ + free(path); + + pthread_rwlock_rdlock(&cg_mount_table_lock); + path = cg_build_path_var(cgroup->name, cg_mount_table[i].name, + NULL); + pthread_rwlock_unlock(&cg_mount_table_lock); + if (!path) continue; - } /* * Get the uid and gid information @@ -1594,6 +1687,7 @@ int cgroup_get_cgroup(struct cgroup *cgr } dir = opendir(path); + free(path); if (!dir) { error = ECGOTHER; goto unlock_error; @@ -1626,6 +1720,8 @@ int cgroup_get_cgroup(struct cgroup *cgr unlock_error: pthread_rwlock_unlock(&cg_mount_table_lock); + if (path) + free(path); /* * XX: Need to figure out how to cleanup? Cleanup just the stuff * we added, or the whole structure. ------------------------------------------------------------------------------ _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
