From: Tom Hromatka <tom.hromatka@oracle.com>
Date: Wed, 22 Feb 2023 21:09:07 +0000 (-0700)
Subject: tools/cgxset: Fix potential memory leak in converted_src_cgroup
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8da741e19bb53a115cbea4e17a9ca8636b7fc435;p=thirdparty%2Flibcgroup.git

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 1a8843af..06495d5e 100644
--- 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;