[lxc-devel] [PATCH] cleanup fds, memory in lxc_cgroup_load_meta2()
Serge Hallyn
serge.hallyn at ubuntu.com
Tue Sep 17 03:20:19 UTC 2013
Quoting Dwight Engen (dwight.engen at oracle.com):
> There are fd leaks in lxc_cgroup_load_meta2() in particular in the success
> case. This change attempts to ensure resources are free'd/close'd, but it
> is possible there are still some error cases where leaks occur.
>
> Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> ---
> Hi Christian,
>
> These changes fix problems I was seeing with valgrind, but they may not be
> how you would do it, so feel free to rework them if I've messed up your
> code too bad :) The command I was using:
> valgrind --leak-check=full --track-fds=yes ./lxc-test-concurrent -i 2 -j 1
Thanks for spotting them. I think it'd be better to split that function
up first. Then it should be easier both to spot the leaks and verify
patches.
I can take a stab at that sometime this week if (either of) you don't
have time.
> src/lxc/cgroup.c | 83 ++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 48 insertions(+), 35 deletions(-)
>
> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> index 9417e77..101998b 100644
> --- a/src/lxc/cgroup.c
> +++ b/src/lxc/cgroup.c
> @@ -124,7 +124,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> size_t token_capacity = 0;
> char *line = NULL;
> size_t sz = 0;
> - int r, saved_errno = 0;
> + int r, ret = ENOMEM;
>
> /* if the subsystem whitelist is not specified, include all
> * hierarchies that contain kernel subsystems by default but
> @@ -149,8 +149,10 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
>
> /* Step 1: determine all kernel subsystems */
> proc_cgroups = fopen_cloexec("/proc/cgroups", "r");
> - if (!proc_cgroups)
> - goto out_error;
> + if (!proc_cgroups) {
> + ret = errno;
> + goto out1;
> + }
>
> while (getline(&line, &sz, proc_cgroups) != -1) {
> char *tab1;
> @@ -179,10 +181,10 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
>
> r = lxc_grow_array((void ***)&kernel_subsystems, &kernel_subsystems_capacity, kernel_subsystems_count + 1, 12);
> if (r < 0)
> - goto out_error;
> + goto out2;
> kernel_subsystems[kernel_subsystems_count] = strdup(line);
> if (!kernel_subsystems[kernel_subsystems_count])
> - goto out_error;
> + goto out2;
> kernel_subsystems_count++;
> }
>
> @@ -198,8 +200,10 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> * /proc/self is not valid, we try /proc/1/cgroup... */
> if (!proc_self_cgroup)
> proc_self_cgroup = fopen_cloexec("/proc/1/cgroup", "r");
> - if (!proc_self_cgroup)
> - goto out_error;
> + if (!proc_self_cgroup) {
> + ret = errno;
> + goto out2;
> + }
>
> while (getline(&line, &sz, proc_self_cgroup) != -1) {
> /* file format: hierarchy:subsystems:group,
> @@ -234,25 +238,25 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> */
> r = lxc_grow_array((void ***)&meta_data->hierarchies, &hierarchy_capacity, hierarchy_number + 1, 12);
> if (r < 0)
> - goto out_error;
> + goto out3;
>
> meta_data->maximum_hierarchy = hierarchy_number;
> }
>
> /* this shouldn't happen, we had this already */
> if (meta_data->hierarchies[hierarchy_number])
> - goto out_error;
> + goto out3;
>
> h = calloc(1, sizeof(struct cgroup_hierarchy));
> if (!h)
> - goto out_error;
> + goto out3;
>
> meta_data->hierarchies[hierarchy_number] = h;
>
> h->index = hierarchy_number;
> h->subsystems = lxc_string_split_and_trim(colon1, ',');
> if (!h->subsystems)
> - goto out_error;
> + goto out3;
> /* see if this hierarchy should be considered */
> if (!all_kernel_subsystems || !all_named_subsystems) {
> for (p = h->subsystems; *p; p++) {
> @@ -283,8 +287,10 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> * /proc/self is not valid, we try /proc/1/cgroup... */
> if (!proc_self_mountinfo)
> proc_self_mountinfo = fopen_cloexec("/proc/1/mountinfo", "r");
> - if (!proc_self_mountinfo)
> - goto out_error;
> + if (!proc_self_mountinfo) {
> + ret = errno;
> + goto out3;
> + }
>
> while (getline(&line, &sz, proc_self_mountinfo) != -1) {
> char *token, *saveptr = NULL;
> @@ -299,7 +305,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> for (i = 0; (token = strtok_r(line, " ", &saveptr)); line = NULL) {
> r = lxc_grow_array((void ***)&tokens, &token_capacity, i + 1, 64);
> if (r < 0)
> - goto out_error;
> + goto out4;
> tokens[i++] = token;
> }
>
> @@ -335,7 +341,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
>
> subsystems = subsystems_from_mount_options(tokens[j + 3], kernel_subsystems);
> if (!subsystems)
> - goto out_error;
> + goto out4;
>
> h = NULL;
> for (k = 1; k <= meta_data->maximum_hierarchy; k++) {
> @@ -352,12 +358,12 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
>
> r = lxc_grow_array((void ***)&meta_data->mount_points, &mount_point_capacity, mount_point_count + 1, 12);
> if (r < 0)
> - goto out_error;
> + goto out4;
>
> /* create mount point object */
> mount_point = calloc(1, sizeof(*mount_point));
> if (!mount_point)
> - goto out_error;
> + goto out4;
>
> meta_data->mount_points[mount_point_count++] = mount_point;
>
> @@ -365,7 +371,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> mount_point->mount_point = strdup(tokens[4]);
> mount_point->mount_prefix = strdup(tokens[3]);
> if (!mount_point->mount_point || !mount_point->mount_prefix)
> - goto out_error;
> + goto out4;
> mount_point->read_only = !lxc_string_in_list("rw", tokens[5], ',');
>
> if (!strcmp(mount_point->mount_prefix, "/")) {
> @@ -381,32 +387,39 @@ struct cgroup_meta_data *lxc_cgroup_load_meta2(const char **subsystem_whitelist)
> k = lxc_array_len((void **)h->all_mount_points);
> r = lxc_grow_array((void ***)&h->all_mount_points, &h->all_mount_point_capacity, k + 1, 4);
> if (r < 0)
> - goto out_error;
> + goto out4;
> h->all_mount_points[k] = mount_point;
> }
>
> /* oops, we couldn't find anything */
> if (!meta_data->hierarchies || !meta_data->mount_points) {
> - errno = EINVAL;
> - goto out_error;
> + ret = EINVAL;
> + goto out4;
> }
>
> - return meta_data;
> -
> -out_error:
> - saved_errno = errno;
> - if (proc_cgroups)
> - fclose(proc_cgroups);
> - if (proc_self_cgroup)
> - fclose(proc_self_cgroup);
> + ret = 0;
> +out4:
> if (proc_self_mountinfo)
> fclose(proc_self_mountinfo);
> - free(line);
> - free(tokens);
> - lxc_free_array((void **)kernel_subsystems, free);
> - lxc_cgroup_put_meta(meta_data);
> - errno = saved_errno;
> - return NULL;
> +out3:
> + if (proc_self_cgroup)
> + fclose(proc_self_cgroup);
> +out2:
> + if (proc_cgroups)
> + fclose(proc_cgroups);
> + if (line)
> + free(line);
> + if (tokens)
> + free(tokens);
> +out1:
> + if (ret != 0) {
> + lxc_free_array((void **)kernel_subsystems, free);
> + lxc_cgroup_put_meta(meta_data);
> + free(meta_data);
> + meta_data = NULL;
> + }
> + errno = ret;
> + return meta_data;
> }
>
> struct cgroup_meta_data *lxc_cgroup_get_meta(struct cgroup_meta_data *meta_data)
> --
> 1.8.1.4
>
>
> ------------------------------------------------------------------------------
> LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
> 1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
> 2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
> Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
> http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel
More information about the lxc-devel
mailing list