diff mbox series

[iproute2,2/2] lib/fs: Fix single return points for get_cgroup2_*

Message ID 9b07b59c4c422b29d6c8297f7f7ec0f2dcc7fb3f.1608315719.git.aclaudi@redhat.com (mailing list archive)
State New, archived
Delegated to: David Ahern
Headers show
Series Some fixes to lib/fs.c | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Andrea Claudi Dec. 18, 2020, 7:09 p.m. UTC 
Functions get_cgroup2_id() and get_cgroup2_path() uncorrectly performs
cleanup on the single return point. Both of them may get to use close()
with a negative argument, if open() fails.

Fix this adding proper labels and gotos to make sure we clean up only
resources we are effectively used before.

Fixes: d5e6ee0dac64 ("ss: introduce cgroup2 cache and helper functions")
Fixes: 8f1cd119b377 ("lib: fix checking of returned file handle size for cgroup")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 lib/fs.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Phil Sutter Dec. 18, 2020, 11:08 p.m. UTC | #1 
On Fri, Dec 18, 2020 at 08:09:23PM +0100, Andrea Claudi wrote:
> Functions get_cgroup2_id() and get_cgroup2_path() uncorrectly performs
> cleanup on the single return point. Both of them may get to use close()
> with a negative argument, if open() fails.
> 
> Fix this adding proper labels and gotos to make sure we clean up only
> resources we are effectively used before.

Since free(NULL) is OK according to POSIX, the fds are initialized to -1
and open() returns -1 on error, you may simplify these
changes down to making the close() calls conditional:

| if (fd >= 0)
| 	close(fd);

Cheers, Phil
Andrea Claudi Dec. 18, 2020, 11:29 p.m. UTC | #2 
On Sat, Dec 19, 2020 at 12:08 AM Phil Sutter <phil@nwl.cc> wrote:
>
> On Fri, Dec 18, 2020 at 08:09:23PM +0100, Andrea Claudi wrote:
> > Functions get_cgroup2_id() and get_cgroup2_path() uncorrectly performs
> > cleanup on the single return point. Both of them may get to use close()
> > with a negative argument, if open() fails.
> >
> > Fix this adding proper labels and gotos to make sure we clean up only
> > resources we are effectively used before.
>
> Since free(NULL) is OK according to POSIX, the fds are initialized to -1
> and open() returns -1 on error, you may simplify these
> changes down to making the close() calls conditional:
>
> | if (fd >= 0)
> |       close(fd);
>
> Cheers, Phil
>

Thanks for the suggestion, Phil. Will do in v2.
diff mbox series

Patch

diff --git a/lib/fs.c b/lib/fs.c
index 2ae506ec..77414e99 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -139,27 +139,31 @@  __u64 get_cgroup2_id(const char *path)
 		mnt_fd = open(mnt, O_RDONLY);
 		if (mnt_fd < 0) {
 			fprintf(stderr, "Failed to open cgroup2 mount\n");
-			goto out;
+			goto out_clean_mnt;
 		}
 
 		fhp->handle_bytes = sizeof(__u64);
 		if (name_to_handle_at(mnt_fd, path, fhp, &mnt_id, 0) < 0) {
 			fprintf(stderr, "Failed to get cgroup2 ID: %s\n",
 					strerror(errno));
-			goto out;
+			goto out_clean_all;
 		}
 	}
 	if (fhp->handle_bytes != sizeof(__u64)) {
 		fprintf(stderr, "Invalid size of cgroup2 ID\n");
-		goto out;
+		if (mnt_fd < 0)
+			goto out;
+		goto out_clean_all;
 	}
 
 	memcpy(cg_id.bytes, fhp->f_handle, sizeof(__u64));
 
-out:
+out_clean_all:
 	close(mnt_fd);
+out_clean_mnt:
 	free(mnt);
 
+out:
 	return cg_id.id;
 }
 
@@ -183,17 +187,17 @@  char *get_cgroup2_path(__u64 id, bool full)
 
 	if (!id) {
 		fprintf(stderr, "Invalid cgroup2 ID\n");
-		return NULL;
+		goto out;
 	}
 
 	mnt = find_cgroup2_mount(false);
 	if (!mnt)
-		return NULL;
+		goto out;
 
 	mnt_fd = open(mnt, O_RDONLY);
 	if (mnt_fd < 0) {
 		fprintf(stderr, "Failed to open cgroup2 mount\n");
-		goto out;
+		goto out_clean_mnt;
 	}
 
 	fhp->handle_bytes = sizeof(__u64);
@@ -203,7 +207,7 @@  char *get_cgroup2_path(__u64 id, bool full)
 	fd = open_by_handle_at(mnt_fd, fhp, 0);
 	if (fd < 0) {
 		fprintf(stderr, "Failed to open cgroup2 by ID\n");
-		goto out;
+		goto out_clean_mntfd;
 	}
 
 	snprintf(fd_path, sizeof(fd_path), "/proc/self/fd/%d", fd);
@@ -212,7 +216,7 @@  char *get_cgroup2_path(__u64 id, bool full)
 		fprintf(stderr,
 			"Failed to read value of symbolic link %s\n",
 			fd_path);
-		goto out;
+		goto out_clean_all;
 	}
 	link_buf[link_len] = '\0';
 
@@ -224,11 +228,14 @@  char *get_cgroup2_path(__u64 id, bool full)
 		fprintf(stderr,
 			"Failed to allocate memory for cgroup2 path\n");
 
-out:
+out_clean_all:
 	close(fd);
+out_clean_mntfd:
 	close(mnt_fd);
+out_clean_mnt:
 	free(mnt);
 
+out:
 	return path;
 }