From 8e0b2dbe02d1ae9836b3dc6cbedb9e9dc77731c7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:43:21 +0000 Subject: [PATCH 1/3] Use PIPE_READ_END, PIPE_WRITE_END to clarify use of privileged op sockets Both sockets in this socket pair are technically bidirectional, but we're using them in a way that is close enough to unidirectional that using these symbolic constants is clearer than magic numbers. We send multi-byte requests into the write end, and read those requests from the read end (even though we also send a 1-byte reply to each request into the "read" end, and read it from the "write" end). Signed-off-by: Simon McVittie --- bubblewrap.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index f8728c7e..d11accba 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -3353,6 +3353,9 @@ main (int argc, pid_t child; int privsep_sockets[2]; + /* We send multi-byte requests into PIPE_WRITE_END and read them from + * PIPE_READ_END, with a 1-byte reply to each request flowing in the + * opposite direction. */ if (socketpair (AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, privsep_sockets) != 0) die_with_error ("Can't create privsep socket"); @@ -3364,8 +3367,8 @@ main (int argc, { /* Unprivileged setup process */ drop_privs (false, true); - close (privsep_sockets[0]); - setup_newroot (opt_unshare_pid, privsep_sockets[1]); + close (privsep_sockets[PIPE_READ_END]); + setup_newroot (opt_unshare_pid, privsep_sockets[PIPE_WRITE_END]); exit (0); } else @@ -3377,8 +3380,8 @@ main (int argc, const char *arg1, *arg2; cleanup_fd int unpriv_socket = -1; - unpriv_socket = privsep_sockets[0]; - close (privsep_sockets[1]); + unpriv_socket = privsep_sockets[PIPE_READ_END]; + close (privsep_sockets[PIPE_WRITE_END]); do { From a76e93f0749cf77a72946a7316e033f78ce82506 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:50:29 +0000 Subject: [PATCH 2/3] utils: Add steal_fd() This is inspired by g_steal_fd() in GLib, and lets us make it explicit that ownership of a fd is being moved, similar to steal_pointer(). Signed-off-by: Simon McVittie --- utils.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/utils.h b/utils.h index 079fe7c9..8fe2d389 100644 --- a/utils.h +++ b/utils.h @@ -199,6 +199,17 @@ steal_pointer (void *pp) #define steal_pointer(pp) \ (0 ? (*(pp)) : (steal_pointer) (pp)) +static inline int +steal_fd (int *fdp) +{ + int fd; + + fd = *fdp; + *fdp = -1; + + return fd; +} + typedef struct _StringBuilder StringBuilder; struct _StringBuilder From 051a67ba46d598a472be353d0c0cc428e4a7b760 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Oct 2024 14:51:28 +0000 Subject: [PATCH 3/3] Clarify ownership of privileged op sockets Setting the members of privsep_sockets[] to -1 when they have been closed or had their ownership transferred is clearer than leaving behind dangling references. Signed-off-by: Simon McVittie --- bubblewrap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index d11accba..8a9f0535 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -3367,8 +3367,8 @@ main (int argc, { /* Unprivileged setup process */ drop_privs (false, true); - close (privsep_sockets[PIPE_READ_END]); - setup_newroot (opt_unshare_pid, privsep_sockets[PIPE_WRITE_END]); + cleanup_fdp (&privsep_sockets[PIPE_READ_END]); + setup_newroot (opt_unshare_pid, steal_fd (&privsep_sockets[PIPE_WRITE_END])); exit (0); } else @@ -3380,8 +3380,8 @@ main (int argc, const char *arg1, *arg2; cleanup_fd int unpriv_socket = -1; - unpriv_socket = privsep_sockets[PIPE_READ_END]; - close (privsep_sockets[PIPE_WRITE_END]); + unpriv_socket = steal_fd (&privsep_sockets[PIPE_READ_END]); + cleanup_fdp (&privsep_sockets[PIPE_WRITE_END]); do {