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.

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

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

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

Conversation

WGH-
Copy link

@WGH- WGH- commented Apr 17, 2023

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

bwrap.xml Outdated Show resolved Hide resolved
@WGH-
Copy link
Author

WGH- commented Jun 10, 2024

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

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 <wgh@torlan.ru>
@WGH-
Copy link
Author

WGH- commented Jun 11, 2024

Rebased

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: --json-status-fd could provide pid of command?
3 participants