[PATCH iproute2 0/2] Some fixes to lib/fs.c

[PATCH iproute2 0/2] Some fixes to lib/fs.c

[Andrea Claudi]

This series contains a couple of fixes and improvements on lib/fs.c
- in functions get_cgroup2_id() and get_cgroup2_path(), fixes cleanup on
  single return point;
- in function make_path(), avoid to call mkdir() two times in a row.

Andrea Claudi (2):
  lib/fs: avoid double call to mkdir on make_path()
  lib/fs: Fix single return points for get_cgroup2_*

 lib/fs.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

-- 
2.29.2

[PATCH iproute2 1/2] lib/fs: avoid double call to mkdir on make_path()

[Andrea Claudi]

make_path() function calls mkdir two times in a row. The first one it
stores mkdir return code, and then it calls it again to check for errno.

This seems unnecessary, as we can use the return code from the first
call and check for errno if not 0.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 lib/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fs.c b/lib/fs.c
index 4b90a704..2ae506ec 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -253,7 +253,7 @@ int make_path(const char *path, mode_t mode)
 			*delim = '\0';
 
 		rc = mkdir(dir, mode);
-		if (mkdir(dir, mode) != 0 && errno != EEXIST) {
+		if (rc && errno != EEXIST) {
 			fprintf(stderr, "mkdir failed for %s: %s\n",
 				dir, strerror(errno));
 			goto out;
-- 
2.29.2

Re: [PATCH iproute2 1/2] lib/fs: avoid double call to mkdir on make_path()

[Phil Sutter]

Hi Andrea,

On Fri, Dec 18, 2020 at 08:09:22PM +0100, Andrea Claudi wrote:
> make_path() function calls mkdir two times in a row. The first one it
> stores mkdir return code, and then it calls it again to check for errno.

To me it rather seems like I rebased the original commit into a mess. Or
I got really confused by the covscan error message this is based upon.
Either way, I don't see why this would not be a bug. :)

> This seems unnecessary, as we can use the return code from the first
> call and check for errno if not 0.
> 

Fixes: ac3415f5c1b1d ("lib/fs: Fix and simplify make_path()")
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
Acked-by: Phil Sutter <phil@nwl.cc>

Thanks, Phil

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

[Andrea Claudi]

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(-)

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;
 }
 
-- 
2.29.2

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

[Phil Sutter]

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

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

[Andrea Claudi]

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.