Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy up handling of intermediate pid fds #665

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Tidy up handling of intermediate pid fds #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e860c0a
Use PIPE_READ_END, PIPE_WRITE_END to clarify use of a socketpair
smcv Oct 30, 2024
d43805b
Close unused ends of intermediate_pids_sockets sooner
smcv Oct 30, 2024
1484a95
Use cleanup_fdp to close intermediate pid sockets
smcv Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3143,12 +3143,12 @@ main (int argc,
if (pid != 0)
{
/* Parent, outside sandbox, privileged (initially) */
cleanup_fdp (&intermediate_pids_sockets[PIPE_WRITE_END]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about assigning -1 to the passe pointer in cleanup_fdp(), so a potential second close() call won't close an uninvolved descriptor.

For the cleanup attribute annotated variables compilers should be able to optimize the assignment out.

Another unrelated thought: systemd uses a safe_close() wrapper instead of directly close(3) to assert EBADF was not returned, to spot file descriptor mixups, see https://github.com/systemd/systemd/blob/v256.7/src/basic/fd-util.c#L59.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about assigning -1 to the passe pointer in cleanup_fdp()

Oh... I thought it already did that, but in fact it doesn't, so this PR needs redoing with either a different function or an augmented version of cleanup_fdp() (as do #666 and #667).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This closes the fd and sets the variable to -1 as a single operation

[citation needed]


if (intermediate_pids_sockets[0] != -1)
if (intermediate_pids_sockets[PIPE_READ_END] != -1)
smcv marked this conversation as resolved.
Show resolved Hide resolved
{
close (intermediate_pids_sockets[1]);
pid = read_pid_from_socket (intermediate_pids_sockets[0]);
close (intermediate_pids_sockets[0]);
pid = read_pid_from_socket (intermediate_pids_sockets[PIPE_READ_END]);
cleanup_fdp (&intermediate_pids_sockets[PIPE_READ_END]);
}

/* Discover namespace ids before we drop privileges */
Expand Down Expand Up @@ -3212,6 +3212,8 @@ main (int argc,
return monitor_child (event_fd, pid, setup_finished_pipe[0]);
}

cleanup_fdp (&intermediate_pids_sockets[PIPE_READ_END]);

if (opt_pidns_fd > 0)
{
if (setns (opt_pidns_fd, CLONE_NEWPID) != 0)
Expand All @@ -3231,10 +3233,8 @@ main (int argc,
}

/* We're back, either in a child or grandchild, so message the actual pid to the monitor */

close (intermediate_pids_sockets[0]);
send_pid_on_socket (intermediate_pids_sockets[1]);
close (intermediate_pids_sockets[1]);
send_pid_on_socket (intermediate_pids_sockets[PIPE_WRITE_END]);
cleanup_fdp (&intermediate_pids_sockets[PIPE_WRITE_END]);
}

/* Child, in sandbox, privileged in the parent or in the user namespace (if --unshare-user).
Expand Down
8 changes: 7 additions & 1 deletion utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,12 @@ send_pid_on_socket (int sockfd)
die_with_error ("Can't send pid");
}

/*
* Create a socket pair such that if one process calls
* send_pid_on_socket(sockets[PIPE_WRITE_END]),
* another process will be able to call
* read_pid_from_socket(sockets[PIPE_READ_END]).
*/
void
create_pid_socketpair (int sockets[2])
{
Expand All @@ -790,7 +796,7 @@ create_pid_socketpair (int sockets[2])
if (socketpair (AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sockets) != 0)
die_with_error ("Can't create intermediate pids socket");

if (setsockopt (sockets[0], SOL_SOCKET, SO_PASSCRED, &enable, sizeof (enable)) < 0)
if (setsockopt (sockets[PIPE_READ_END], SOL_SOCKET, SO_PASSCRED, &enable, sizeof (enable)) < 0)
die_with_error ("Can't set SO_PASSCRED");
}

Expand Down
Loading