Some fixes to lib/fs.c

PATCH Superseded Andrea Claudi 2 participants 6 messages Dec 18, 2020

Subsystems (3)

Networking Network drivers Library code

lore

Share & tools

lore

Andrea Claudi

author

2020-12-18 19:16 UTC #0 lore raw

cover Some fixes to lib/fs.c

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

Signature · 1 line

2.29.2

Andrea Claudi

author

2020-12-18 19:16 UTC #1 lore raw

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

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 <[email protected]>

---
 lib/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/fs.c +1 -1

index 4b90a704..2ae506ec 100644
@@ -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;

Signature · 1 line

2.29.2

Phil Sutter

Acked-by on v1 1/2

2020-12-18 23:00 UTC #2 lore raw

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

Hi Andrea,

Quoted 2 lines · 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.

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. :)

Quoted 3 lines

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

Quoted 1 line

Signed-off-by Andrea Claudi <[email protected]>

Acked-by Phil Sutter <[email protected]>

Thanks, Phil

Andrea Claudi

author

2020-12-18 19:16 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 lib/fs.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

lib/fs.c +17 ~~-10~~

index 2ae506ec..77414e99 100644
@@ -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;
 }

Signature · 1 line

2.29.2

Phil Sutter

2020-12-18 23:09 UTC #4 lore raw

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

Quoted 6 lines · 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.

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

author

2020-12-18 23:30 UTC #5 lore raw

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

Quoted 9 lines · Phil Sutter

Quoted 6 lines · 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.

Quoted 9 lines

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.