tools/cgxset: Fix potential memory leak in converted_src_cgroup

There's a brief window where converted_src_cgroup is allocated but
hasn't been assigned to the *cgroup pointer.  If a failure occurs in
this window, then converted_src_cgroup must be freed before doing the
goto to the error path.

Also, simplify the error-handling logic in cgxset::main() by having all
errors goto "err".

Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
Signed-off-by: Tom Hromatka <tom.hromatka@oracle.com>
(cherry picked from commit f1579cec2734c767d24f69944e1e5f07e1c1e3af)

diff --git a/src/tools/cgxset.c b/src/tools/cgxset.c
index 1a8843af6bd10aa713afb15d4d07c8bdc0ae632e..06495d5ea1a1350373ebbfdf10b13442d8706e75 100644 (file)
--- a/src/tools/cgxset.c
+++ b/src/tools/cgxset.c
@@ -272,7 +272,7 @@ int main(int argc, char *argv[])
                if (!cgroup) {
                        ret = ECGFAIL;
                        err("%s: can't add new cgroup: %s\n", argv[0], cgroup_strerror(ret));
-                       goto cgroup_free_err;
+                       goto err;
                }
 
                /* copy the values from the source cgroup to new one */
@@ -280,7 +280,7 @@ int main(int argc, char *argv[])
                if (ret != 0) {
                        err("%s: cgroup %s error: %s\n", argv[0], src_cg_path,
                            cgroup_strerror(ret));
-                       goto cgroup_free_err;
+                       goto err;
                }
 
                converted_src_cgroup = cgroup_new_cgroup(cgroup->name);
@@ -289,6 +289,11 @@ int main(int argc, char *argv[])
                        goto err;
                }
 
+               /*
+                * If a failure occurs between this comment and the "End of above comment"
+                * below, then we need to manually free converted_src_cgroup.
+                */
+
                ret = cgroup_convert_cgroup(converted_src_cgroup, CGROUP_DISK, src_cgroup,
                                            src_version);
                if (ret == ECGNOVERSIONCONVERT && ignore_unmappable)
@@ -298,8 +303,14 @@ int main(int argc, char *argv[])
                         * v1 to v2 or vice versa
                         */
                        ret = 0;
-               else if (ret)
+               else if (ret) {
+                       free(converted_src_cgroup);
                        goto err;
+               }
+
+               /*
+                * End of above comment about converted_src_cgroup needing to be manually freed.
+                */
 
                cgroup_free(&cgroup);
                cgroup = converted_src_cgroup;
@@ -308,19 +319,18 @@ int main(int argc, char *argv[])
                ret = cgroup_modify_cgroup(cgroup);
                if (ret) {
                        err("%s: cgroup modify error: %s\n", argv[0], cgroup_strerror(ret));
-                       goto cgroup_free_err;
+                       goto err;
                }
 
                optind++;
                cgroup_free(&cgroup);
        }
 
-cgroup_free_err:
+err:
        if (cgroup)
                cgroup_free(&cgroup);
        if (src_cgroup)
                cgroup_free(&src_cgroup);
-err:
        free(name_value);
 
        return ret;