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

Jump to bottom

Add command-pid member to --json-status-fd #576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add command-pid member to --json-status-fd #576

wants to merge 1 commit into from

Conversation

WGH-
Copy link

@WGH- WGH- commented Apr 17, 2023 •
edited
Loading

In cases where bwrap runs its own additional PID 1 process (e.g. --unshare-pid), child-pid is the PID of that process, not the PID of the user-specified COMMAND.

This is inconvenient as the user might want to send e.g. SIGTERM to the command to give it a chance to exit gracefully.

Example:

$ ./bwrap --json-status-fd 2 -- /bin/true
{ "child-pid": 2779366, "mnt-namespace": 4026538517 }
{ "command-pid": 2779366 }

$ ./bwrap --json-status-fd 2 --unshare-pid -- /bin/true
{ "child-pid": 2779372, "mnt-namespace": 4026538517, "pid-namespace": 4026538518 }
{ "command-pid": 2779373 }

Closes #553

@WGH-
Copy link
Author

WGH- commented Apr 17, 2023

Note that unlike child-pid this is currently impossible to use while being immune to PID reuse.

For child-pid you can do something like this:

pid = receive_pid_from_json_status_fd()
pidfd = pidfd_open(fd)
# if block_fd is not closed, then child-pid certainly was alive at the time of pidfd_open() call
if closed(block_fd):
    close(pidfd)
    return None
return pidfd

Unless I'm missing some simple solution, we'll have to introduce another block_fd-like fd that would be leaked to the COMMAND process (and even then COMMAND may close it early, making the code inadvertently fail).

mcatanzaro
mcatanzaro reviewed Jun 10, 2024
View reviewed changes
bwrap.xml Outdated Show resolved Hide resolved
@mcatanzaro mcatanzaro mentioned this pull request Jun 10, 2024
Open
@WGH-
Copy link
Author

WGH- commented Jun 10, 2024

Heh. I'll rebase and fix conflicts in a day or so.

@WGH-
Add command-pid member to --json-status-fd
4b2f32f 
In cases where bwrap runs its own additional PID 1 process (e.g.
--unshare-pid), child-pid is the PID of that process, not the
PID of the user-specified COMMAND.

This is inconvenient as the user might want to send e.g. SIGTERM to the
command to give it a chance to exit gracefully.

Closes containers#553

Signed-off-by: WGH <[email protected]>
@WGH-
Copy link
Author

WGH- commented Jun 11, 2024

Rebased

@mcatanzaro mcatanzaro mentioned this pull request Jun 13, 2024
Merged
smcv
smcv requested changes Oct 30, 2024
View reviewed changes
bubblewrap.c
Comment on lines +2999 to +3007
close (command_pid_sockets[1]);
command_pid = read_pid_from_socket (command_pid_sockets[0]);
close (command_pid_sockets[0]);

if (opt_json_status_fd != -1)
{
cleanup_free char *output = xasprintf ("{ \"command-pid\": %i }\n", command_pid);
dump_info (opt_json_status_fd, output, TRUE);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will make the uppermost process (the monitor process, outside the sandbox) block until the final process sends its pid. And, if the child process (reported as child-pid) fails for any reason, then read_pid_from_socket() will fail with a fatal error, preventing monitor_child() from relaying the true exit status of the child.

I think it would be better to monitor command_pid_sockets[0] in monitor_child(), and use non-blocking I/O for that, so that we output the information when we have it, but we don't wait for it.

@smcv
Copy link
Collaborator

smcv commented Oct 30, 2024 •
edited
Loading

currently impossible to use while being immune to PID reuse

Yeah, that's quite a serious limitation. It doesn't necessarily make this feature addition entirely useless, but it's certainly something that users of this feature would need to be careful about.

This is inconvenient as the user might want to send e.g. SIGTERM to the command to give it a chance to exit gracefully

I think the better way to solve this particular use-case would be a fixed version of #586: teach the top-level bwrap monitor process to be able to forward signals like SIGTERM to the sandbox init process (already done in #586), and then teach the sandbox init process (do_init()), if used, to forward those same signals into the final process that exec'd the user-specified COMMAND.

That way, instead of sending your signals to the child-pid (possibly subject to PID reuse bugs) or to the command-pid (definitely subject to PID reuse bugs), you could send them to the top-level bwrap monitor, which would forward them to the correct child process internally.

@smcv smcv mentioned this pull request Oct 30, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcatanzaro mcatanzaro mcatanzaro left review comments

@smcv smcv smcv requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: --json-status-fd could provide pid of command?
3 participants
@WGH- @smcv @mcatanzaro