From 2ae2ec35424a84d3f6baaa114f36d9a2a88962bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Feb 2023 21:22:37 +0100 Subject: Enable and resolve sign comparisson warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comparisson of different signedness can result in unexpected results due to implicit conversions. ../network.c:81:34: warning: comparison of integer expressions of different signedness: ‘__u32’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 81 | if (rheader->nlmsg_seq != seq_nr) | ^~ ../network.c:83:34: warning: comparison of integer expressions of different signedness: ‘__u32’ {aka ‘unsigned int’} and ‘__pid_t’ {aka ‘int’} [-Wsign-compare] 83 | if (rheader->nlmsg_pid != getpid ()) | ^~ ../bind-mount.c:268:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare] 268 | assert (i < n_lines); | ^ ../bind-mount.c:309:13: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare] 309 | assert (i == n_lines); | ^~ ../bind-mount.c:318:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare] 318 | for (i = 0; i < n_lines; i++) | ^ ../bind-mount.c:321:17: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare] 321 | for (i = 0; i < n_lines; i++) | ^ ../utils.c:818:19: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘ssize_t’ {aka ‘long int’} [-Wsign-compare] 818 | while (size - 2 < n); | ^ ../bubblewrap.c:489:13: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] 489 | assert (j < sizeof(dont_close)/sizeof(*dont_close)); | ^ ../bubblewrap.c:994:25: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uid_t’ {aka ‘unsigned int’} [-Wsign-compare] 994 | if (setfsuid (-1) != real_uid) | ^~ ../bubblewrap.c:1042:61: warning: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare] 1042 | if (write (privileged_op_socket, buffer, buffer_size) != buffer_size) | ^~ ../bubblewrap.c:1232:25: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] 1232 | for (i = 0; i < N_ELEMENTS (cover_proc_dirs); i++) | ^ ../bubblewrap.c:1260:25: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] 1260 | for (i = 0; i < N_ELEMENTS (devnodes); i++) | ^ ../bubblewrap.c:1272:25: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] 1272 | for (i = 0; i < N_ELEMENTS (stdionodes); i++) | ^ ../bubblewrap.c: In function ‘read_priv_sec_op’: ../bubblewrap.c:1556:15: warning: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘long unsigned int’ [-Wsign-compare] 1556 | if (rec_len < sizeof (PrivSepOp)) | ^ ../bubblewrap.c:1626:28: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare] 1626 | if (*total_parsed_argc_p > MAX_ARGS) | ^ ../bubblewrap.c:1681:40: warning: comparison of integer expressions of different signedness: ‘int’ and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare] 1681 | if (*total_parsed_argc_p > MAX_ARGS) | ^ ../bubblewrap.c:2265:31: warning: comparison of integer expressions of different signedness: ‘uid_t’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 2265 | if (opt_sandbox_uid != -1) | ^~ ../bubblewrap.c:2285:31: warning: comparison of integer expressions of different signedness: ‘gid_t’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 2285 | if (opt_sandbox_gid != -1) | ^~ ../bubblewrap.c:2678:23: warning: comparison of integer expressions of different signedness: ‘uid_t’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 2678 | if (opt_sandbox_uid == -1) | ^~ ../bubblewrap.c:2680:23: warning: comparison of integer expressions of different signedness: ‘gid_t’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 2680 | if (opt_sandbox_gid == -1) | ^~ Signed-off-by: Christian Göttsche --- bind-mount.c | 2 +- bubblewrap.c | 20 ++++++++++---------- meson.build | 5 ----- network.c | 6 +++--- utils.c | 2 +- 5 files changed, 15 insertions(+), 20 deletions(-) diff --git a/bind-mount.c b/bind-mount.c index 488d85a..57b4236 100644 --- a/bind-mount.c +++ b/bind-mount.c @@ -237,7 +237,7 @@ parse_mountinfo (int proc_fd, MountInfo *end_tab; int n_mounts; char *line; - int i; + unsigned int i; int max_id; unsigned int n_lines; int root; diff --git a/bubblewrap.c b/bubblewrap.c index 8322ea0..97f12af 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -496,7 +496,7 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd) int num_fds; struct signalfd_siginfo fdsi; int dont_close[] = {-1, -1, -1, -1}; - int j = 0; + unsigned int j = 0; int exitc; pid_t died_pid; int died_status; @@ -1014,7 +1014,7 @@ write_uid_gid_map (uid_t sandbox_uid, if (is_privileged) { setfsuid (old_fsuid); - if (setfsuid (-1) != real_uid) + if ((uid_t) setfsuid (-1) != real_uid) die ("Unable to re-set fsuid"); } } @@ -1065,7 +1065,7 @@ privileged_op (int privileged_op_socket, if (arg2 != NULL) strcpy ((char *) buffer + arg2_offset, arg2); - if (write (privileged_op_socket, buffer, buffer_size) != buffer_size) + if (write (privileged_op_socket, buffer, buffer_size) != (ssize_t)buffer_size) die ("Can't write to privileged_op_socket"); if (read (privileged_op_socket, buffer, 1) != 1) @@ -1182,7 +1182,7 @@ setup_newroot (bool unshare_pid, cleanup_free char *source = NULL; cleanup_free char *dest = NULL; int source_mode = 0; - int i; + unsigned int i; if (op->source && op->type != SETUP_MAKE_SYMLINK) @@ -1593,7 +1593,7 @@ read_priv_sec_op (int read_socket, if (rec_len == 0) exit (1); /* Privileged helper died and printed error, so exit silently */ - if (rec_len < sizeof (PrivSepOp)) + if ((size_t)rec_len < sizeof (PrivSepOp)) die ("Invalid size %zd from unprivileged helper", rec_len); /* Guarantee zero termination of any strings */ @@ -1647,7 +1647,7 @@ parse_args_recurse (int *argcp, * I picked 9000 because the Internet told me to and it was hard to * resist. */ - static const uint32_t MAX_ARGS = 9000; + static const int32_t MAX_ARGS = 9000; if (*total_parsed_argc_p > MAX_ARGS) die ("Exceeded maximum number of arguments %u", MAX_ARGS); @@ -2300,7 +2300,7 @@ parse_args_recurse (int *argcp, if (argc < 2) die ("--uid takes an argument"); - if (opt_sandbox_uid != -1) + if (opt_sandbox_uid != (uid_t)-1) warn_only_last_option ("--uid"); the_uid = strtol (argv[1], &endptr, 10); @@ -2320,7 +2320,7 @@ parse_args_recurse (int *argcp, if (argc < 2) die ("--gid takes an argument"); - if (opt_sandbox_gid != -1) + if (opt_sandbox_gid != (gid_t)-1) warn_only_last_option ("--gid"); the_gid = strtol (argv[1], &endptr, 10); @@ -2768,9 +2768,9 @@ main (int argc, __debug__ (("Creating root mount point\n")); - if (opt_sandbox_uid == -1) + if (opt_sandbox_uid == (uid_t)-1) opt_sandbox_uid = real_uid; - if (opt_sandbox_gid == -1) + if (opt_sandbox_gid == (gid_t)-1) opt_sandbox_gid = real_gid; if (!opt_unshare_user && opt_userns_fd == -1 && opt_sandbox_uid != real_uid) diff --git a/meson.build b/meson.build index 6de2bac..0c26aa5 100644 --- a/meson.build +++ b/meson.build @@ -37,11 +37,6 @@ add_project_arguments( '-Werror=switch-default', '-Wswitch-enum', - # Meson warning_level=2 would do this, but we are not fully - # signedness-safe yet - '-Wno-sign-compare', - '-Wno-error=sign-compare', - # Deliberately not warning about these, ability to zero-initialize # a struct is a feature '-Wno-missing-field-initializers', diff --git a/network.c b/network.c index 9477cb7..b47965c 100644 --- a/network.c +++ b/network.c @@ -62,8 +62,8 @@ rtnl_send_request (int rtnl_fd, } static int -rtnl_read_reply (int rtnl_fd, - int seq_nr) +rtnl_read_reply (int rtnl_fd, + unsigned int seq_nr) { char buffer[1024]; ssize_t received; @@ -80,7 +80,7 @@ rtnl_read_reply (int rtnl_fd, { if (rheader->nlmsg_seq != seq_nr) return -1; - if (rheader->nlmsg_pid != getpid ()) + if ((pid_t)rheader->nlmsg_pid != getpid ()) return -1; if (rheader->nlmsg_type == NLMSG_ERROR) { diff --git a/utils.c b/utils.c index 693273b..1353ea6 100644 --- a/utils.c +++ b/utils.c @@ -815,7 +815,7 @@ readlink_malloc (const char *pathname) if (n < 0) return NULL; } - while (size - 2 < n); + while (size - 2 < (size_t)n); value[n] = 0; return steal_pointer (&value); -- cgit v1.2.1 From 15ef38c4e3af506e7f35b9372cf9f657e9861701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Feb 2023 21:22:46 +0100 Subject: Avoid implicit conversions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running under pedantic UBSAN: ../bubblewrap.c:968:21: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uid_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned) ../bubblewrap.c:1210:28: runtime error: implicit conversion from type 'int' of value -41 (32-bit, signed) to type 'unsigned int' changed the value to 4294967255 (32-bit, unsigned) ../bubblewrap.c:1215:28: runtime error: implicit conversion from type 'int' of value -41 (32-bit, signed) to type 'unsigned int' changed the value to 4294967255 (32-bit, unsigned) Signed-off-by: Christian Göttsche --- bubblewrap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 97f12af..712b07f 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -965,7 +965,7 @@ write_uid_gid_map (uid_t sandbox_uid, cleanup_free char *gid_map = NULL; cleanup_free char *dir = NULL; cleanup_fd int dir_fd = -1; - uid_t old_fsuid = -1; + uid_t old_fsuid = (uid_t)-1; if (pid == -1) dir = xstrdup ("self"); @@ -1207,12 +1207,12 @@ setup_newroot (bool unshare_pid, * inaccessible by that group. */ if (op->perms >= 0 && (op->perms & 0070) == 0) - parent_mode &= ~0050; + parent_mode &= ~0050U; /* The same, but for users other than the owner and group. */ if (op->perms >= 0 && (op->perms & 0007) == 0) - parent_mode &= ~0005; + parent_mode &= ~0005U; dest = get_newroot_path (op->dest); if (mkdir_with_parents (dest, parent_mode, FALSE) != 0) -- cgit v1.2.1 From 88004f880a7acfe9d8b3016b173b39b24d70bc92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Feb 2023 21:22:48 +0100 Subject: Drop unnecessary cast to same type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Christian Göttsche --- network.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network.c b/network.c index b47965c..f6d58a6 100644 --- a/network.c +++ b/network.c @@ -130,7 +130,7 @@ rtnl_setup_request (char *buffer, header->nlmsg_seq = counter++; header->nlmsg_pid = getpid (); - return (struct nlmsghdr *) header; + return header; } void -- cgit v1.2.1 From fe0da5d368d1c681be684a8fddffaa2cc6587fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Feb 2023 21:22:52 +0100 Subject: Use mode_t as parameter type in mkdir_with_parents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parameter mode only usage is it being passed to ensure_dir(), which takes it as mode_t. Signed-off-by: Christian Göttsche --- utils.c | 2 +- utils.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/utils.c b/utils.c index 1353ea6..7743c2e 100644 --- a/utils.c +++ b/utils.c @@ -672,7 +672,7 @@ ensure_dir (const char *path, /* Sets errno on error (!= 0) */ int mkdir_with_parents (const char *pathname, - int mode, + mode_t mode, bool create_last) { cleanup_free char *fn = NULL; diff --git a/utils.h b/utils.h index 37d8c7c..9191ecc 100644 --- a/utils.h +++ b/utils.h @@ -115,7 +115,7 @@ int ensure_dir (const char *path, mode_t mode); int get_file_mode (const char *pathname); int mkdir_with_parents (const char *pathname, - int mode, + mode_t mode, bool create_last); void create_pid_socketpair (int sockets[2]); void send_pid_on_socket (int socket); -- cgit v1.2.1 From ab5eb5c3f5bc75ee38d66a0c4d56d8a05be54b76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Tue, 28 Feb 2023 21:22:55 +0100 Subject: Declare file local variables static MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Christian Göttsche --- bubblewrap.c | 58 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 712b07f..de06305 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -72,35 +72,35 @@ static const char *opt_exec_label = NULL; static const char *opt_file_label = NULL; static bool opt_as_pid_1; -const char *opt_chdir_path = NULL; -bool opt_assert_userns_disabled = FALSE; -bool opt_disable_userns = FALSE; -bool opt_unshare_user = FALSE; -bool opt_unshare_user_try = FALSE; -bool opt_unshare_pid = FALSE; -bool opt_unshare_ipc = FALSE; -bool opt_unshare_net = FALSE; -bool opt_unshare_uts = FALSE; -bool opt_unshare_cgroup = FALSE; -bool opt_unshare_cgroup_try = FALSE; -bool opt_needs_devpts = FALSE; -bool opt_new_session = FALSE; -bool opt_die_with_parent = FALSE; -uid_t opt_sandbox_uid = -1; -gid_t opt_sandbox_gid = -1; -int opt_sync_fd = -1; -int opt_block_fd = -1; -int opt_userns_block_fd = -1; -int opt_info_fd = -1; -int opt_json_status_fd = -1; -int opt_seccomp_fd = -1; -const char *opt_sandbox_hostname = NULL; -char *opt_args_data = NULL; /* owned */ -int opt_userns_fd = -1; -int opt_userns2_fd = -1; -int opt_pidns_fd = -1; -int next_perms = -1; -size_t next_size_arg = 0; +static const char *opt_chdir_path = NULL; +static bool opt_assert_userns_disabled = FALSE; +static bool opt_disable_userns = FALSE; +static bool opt_unshare_user = FALSE; +static bool opt_unshare_user_try = FALSE; +static bool opt_unshare_pid = FALSE; +static bool opt_unshare_ipc = FALSE; +static bool opt_unshare_net = FALSE; +static bool opt_unshare_uts = FALSE; +static bool opt_unshare_cgroup = FALSE; +static bool opt_unshare_cgroup_try = FALSE; +static bool opt_needs_devpts = FALSE; +static bool opt_new_session = FALSE; +static bool opt_die_with_parent = FALSE; +static uid_t opt_sandbox_uid = -1; +static gid_t opt_sandbox_gid = -1; +static int opt_sync_fd = -1; +static int opt_block_fd = -1; +static int opt_userns_block_fd = -1; +static int opt_info_fd = -1; +static int opt_json_status_fd = -1; +static int opt_seccomp_fd = -1; +static const char *opt_sandbox_hostname = NULL; +static char *opt_args_data = NULL; /* owned */ +static int opt_userns_fd = -1; +static int opt_userns2_fd = -1; +static int opt_pidns_fd = -1; +static int next_perms = -1; +static size_t next_size_arg = 0; #define CAP_TO_MASK_0(x) (1L << ((x) & 31)) #define CAP_TO_MASK_1(x) CAP_TO_MASK_0(x - 32) -- cgit v1.2.1