Add --foreground option #588
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This option creates a new process group for the child process and if there's a controlling terminal, makes the new process group the foreground process group of the controlling terminal. This makes sure that SIGINT signals generated by CTRL+C are only sent to the child process and not to bubblewrap itself. While this can also be achieved with external utilities (e.g. util-linux/util-linux#2445), it still makes sense to support this natively in bubblewrap to avoid the need to install any other utilities in the sandbox to be able to make the child process the foreground process group of the controlling terminal. Fixes containers#369 Signed-off-by: Daan De Meyer <[email protected]>
DaanDeMeyer
force-pushed
the
foreground
branch
from
a2d8edc to
fcd1ff2
Compare
smcv
requested changes
Feb 5, 2024
| @@ -3276,6 +3282,30 @@ main (int argc, | |||
| setsid () == (pid_t) -1) | |||
| die_with_error ("setsid"); | |||
|
|
|||
| if (opt_foreground) { | |||
There was a problem hiding this comment.
Please use GNU-style indentation:
if (opt_foreground)
{
cleanup_fd int tty_fd = -1;
...
| if (sigprocmask (SIG_BLOCK, &mask, &old_mask) == -1) | ||
| die_with_error ("sigprocmask"); | ||
|
|
||
| if (tcsetpgrp (tty_fd, getpgid(0)) == -1) |
There was a problem hiding this comment.
Space before opening parenthesis:
Suggested change
| if (tcsetpgrp (tty_fd, getpgid(0)) == -1) | |
| if (tcsetpgrp (tty_fd, getpgid (0)) == -1) |
| @@ -469,6 +469,14 @@ | |||
| (see CVE-2017-5226). | |||
| </para></listitem> | |||
| </varlistentry> | |||
| <varlistentry> | |||
| <term><option>--foreground</option></term> | |||
There was a problem hiding this comment.
I wonder whether this should have a name more like --terminal-foreground or --tty-foreground, to indicate that it isn't to do with daemonization or other things that might be referred to as "foreground" or "background"?
Comment on lines
+475
to
+477
| Create a new process group for the sandbox (calls setpgid()) and make it the | ||
| foreground process group of the controlling terminal if there is a | ||
| controlling terminal. |
There was a problem hiding this comment.
This seems like it's missing a mention of why this is useful to do things like resolving #369. Perhaps something like this:
Suggested change
| Create a new process group for the sandbox (calls setpgid()) and make it the | |
| foreground process group of the controlling terminal if there is a | |
| controlling terminal. | |
| Create a new process group for the sandbox, and make it the | |
| foreground process group of the controlling terminal if there is a | |
| controlling terminal. This means that keyboard shortcuts that send | |
| signals, such as Ctrl+C for SIGINT or Ctrl+\ for SIGQUIT, will send | |
| the signal to the sandboxed process instead of to bubblewrap itself. |
|
It turns out that for my use case reimplementing the bits I need from bubblewrap is easier than trying to fix the tool to support my use case so I'm closing this as I no longer need it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This option creates a new process group for the child process and if there's a controlling terminal, makes the new process group the foreground process group of the controlling terminal. This makes sure that SIGINT signals generated by CTRL+C are only sent to the child process and not to bubblewrap itself.
While this can also be achieved with external utilities (e.g. util-linux/util-linux#2445), it still makes sense to support this natively in bubblewrap to avoid the need to install any other utilities in the sandbox to be able to make the child process the foreground process group of the controlling terminal.
Fixes #369