Add CLI option to pass common signals to sandboxed process #586
Conversation
aaruni96
changed the title
|
Thanks for the review, and sorry for the shoddy work. I've addressed the parts of review I could in code. This PR relied on the work from #402 , and I had just added a command line option to switch the behaviour on or off.
I don't really know what the intention was, I only tested that the work from the previous author solves the symptom I was facing. I have renamed the function (and reworked other things based on your review) , but haven't edited the comment. I will be happy to rework more things based on another review. Also, do you think PR #588 is a better fix for issue #369 ? Is it worth spending time on this PR to make it up to shape ? |
|
It's not clear to me either way - I am not as expert in Unix minutiae as the primary authors of bubblewrap, but I'm trying to get some reviews done in the other maintainers' absence. I think #588 is probably a better fix for #369 as originally stated, which can be summarized as "I want Ctrl+C to do the normal thing in an interactive terminal". But, explicitly forwarding signals as done here might well be a better approach when bubblewrap is used as an implementation detail of GUI or background processes (most prominently Flatpak, but also Steam, WebKitGTK, various build environments in NixOS, and whatever other situations people put it into), so that a I think the fact that there are two implementations, apparently equally viable but rather different, confirms my belief that neither of these two approaches should be the default.
If this is something that you're interested in making happen, then yes I think so. I'd prefer to review this (and almost any other contribution, really) as a series rebased onto the current bubblewrap
Please amend any commits that are not all your own work to set the original author or add an appropriate |
This, please? In this case I think it would probably be a single commit, Co-authored-by you and the author of #402. |
|
I'd really like to see some automated test coverage for this - perhaps running a shell with a |
I thought I already added the original author of #402 as a co author in commit 225ca09 ? |
You did. What I'm now asking for is combining the initial "not quite right" version with your later fixes, into a single commit that is as correct as we can get it - so that when a future maintainer digs back through the history to find out why something is the way it is, they don't have to look at the initial commit, think "but surely this is wrong?", and then mentally combine it with the commits that followed it to get a better picture of the change that was actually applied. The intermediate stages that your change went through as a result of reviews and revisions are part of the history of this PR, but they don't need to be part of the history of bubblewrap. (It's often easier to review a complete/correct commit that makes sense on its own, too.) Sometimes it does make sense to contribute a PR with more than one commit, but ideally each prefix of the series of commits should be something that would have been valid to apply on its own - like for example #488 adds two related features, but if I had stopped after the first one, that would have been valid (although less useful). For simpler changes like this one, one commit is often enough. |
|
I have addressed the parts of the code review that I could, and left a question for the one I didn't understand. I will squash all my commits into a single co-authored commit after I have included the one remaining point as well. |
smcv
changed the title
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
bubblewrap does not support forwarding signals yet, see containers/bubblewrap#586. As a workaround, we need to make sure we send our signals to the inner process. To make this work, we create a pipe, pass it through to the subprocess, and prefix with a bash command that writes its pid to the pipe before exec-ing the actual command. The other thing we get from this is that we can register the inner pid as a scope which makes the systemctl status output for the scopes we create a lot more useful.
|
I have tested this patchset (backported to 0.8.0), and it unfortunately does not work for Ctrl-C. :( |
|
Can you share exactly what you tried? My branch reports version 0.8.0 as well , but there may be other changes incorporated into master since then till the point I branched off. The last commit shared between my branch and master is ad76c2d, if you want to try from there (or just try cloning my work?). |
|
Took the v0.8.0 tag, applied https://github.com/containers/bubblewrap/pull/586.patch, resolved the trivial conflict, built and tested with gdb. Ctrl-C kills the entire gdb program rather than merely printing "Quit" and giving me back the gdb prompt as normally gdb does. |
Sorry, it took me 8 months to figure out what you mean. I still want to keep 2 commits, just so I know which changes are mine, and which changes I have inherited, if you think that makes sense. |
aaruni96
force-pushed
the
ak/sigint
branch
2 times, most recently
from
3e88c04 to
9fe7aeb
Compare
|
@luke-jr I am not able to reproduce this using the demo script. In fact, the demo script prints "Quit" in gdb with or without |
Sorry, no, I don't think the original ("inherited") change is correct on its own - which is why I asked for changes to be made to it, which you have done. But that also means that the original change is not a useful thing to have in bubblewrap's history, because we know it's incomplete ("wrong", you could say) until after your extra fixes get applied to it. The main consumer for the commit history is a future maintainer doing archaeology on bubblewrap's history to find out why the code is the way it is, in order to identify whether some suspicious code is a bug or actually something necessary - possibly in a hurry, because they might be solving a security vulnerability. When doing that, they are not going to be interested in any false starts and dead ends that happened when this change was passed between contributors: they will only be interested in the final, "correct" version that got accepted. I think you've rewritten this commit enough that it should probably end up looking something like this: |
There was a problem hiding this comment.
The implementation looks reasonable to me.
Regarding the commit message:
Adds an command line switch to optionally pass SIGINT and SIGTERM to the sandboxed process to handle.
What I should have asked when you first opened this PR is: why?
#369 alludes to a few use-cases for this, but the commit description in this PR never actually mentioned any of them, or the bug number #369.
One use-case for this seems to be that if you are using bubblewrap as a wrapper around a program that you ran from a terminal (for example a Flatpak app, or a smaller/simpler sandboxing framework), you will typically expect Ctrl+C to end up sending SIGINT to the program, but until this feature was added, some other thing would happen. (What other thing? It's not 100% clear to me, and probably depends on whether you told bubblewrap to create a new pid namespace or not.)
Another use-case for this seems to have been that if you are using bubblewrap as a wrapper around a daemon, for example a systemd service, you might expect that sending SIGHUP to the "main" process will cause the daemon to reload - but this didn't actually happen that way, because the "main" process is actually bubblewrap's monitor process outside the sandbox process, rather than the in-sandbox program that was specified as the COMMAND on bubblewrap's command-line.
Future maintainers are going to need to know this sort of information in order to understand the intent of the code.
And, in a previous review I wrote:
I'd really like to see some automated test coverage for this - perhaps running a shell with a trap on SIGUSR1 or some such as a child of a background bwrap, killing the bwrap with that signal, asserting that the process is still running and has received the signal (maybe the trap could write to a temporary file or similar), and then killing the bwrap with SIGTERM to clean up.
I would still like to see this automated test coverage added. It shouldn't be particularly difficult to do, and anything that we don't routinely test is very likely to regress.
|
Does this PR resolve #573? If yes, also please add Any other issues that it resolves can have a similar |
|
I will get back to working on this during the coming week, but short summary of where we stand : This PR fixes my symptom of "CTRL+C interrupts bubblewrap, and not the program running inside it (in my case, a julia REPL session)". But this PR doesn't really forward signals to the inside process, it just blocks bubblewrap from getting the signals (or something similar, I don't fully understand). In particular, writing a simple trap script for SIGUSR1 in bubblewrap doesn't get any signals when SIGUSR1 is issued to the bwrap PID, but it gets a signal is SIGUSR1 is issued to the PID of I don't know why this PR solves my symptom, but I will get digging in the work week. |
|
All right, I am thoroughly confused. My original problem (hitting CTRL+C in a julia REPL) is not reproducible with or without this PR. You can test this PR does not really forward signals to child processes by manually issuing a kill signal instead of CTRL+C and trying to trap this in a child of bwrap. On the other hand, this PR does solve the problem indicated in #369 , so clearly, this PR does something desirable. |
|
I have narrowed it down to |
|
Would probably need to use pidfds and pidfd_send_signal(2) instead to work in this case. |
|
Two things are relevant here:
|
This fixes, for example, pressing CTRL+C to send a SIGINT during interactive use and also allows for things like passing SIGWINCH to neovim running inside the bubblewrap to resize the window. Extends [bubblewrap] Propagate SIGTERM and SIGINT to child containers#402 Borrows test from q3cpma Co-authored-by: Earl Chew <[email protected]> Co-authored-by: q3cpma <[email protected]> Signed-off-by: Aaruni Kaushik <[email protected]>
|
In the latest round of changes, I have squashed everything into a single commit, added the "fixes" hint in the original message of the PR, and added tests (thanks @q3cpma ). State of affairs: it works except if |
|
Your test doesn't require bash in any way (now that POSIX-2024 has |
|
I chose bash to keep it in line with the other shell tests. I don't have an opinion one way or the other, I am happy to change it to whatever the maintainers prefer. |
Adds an command line switch to optionally pass SIGINT and SIGTERM to the sandboxed process to handle.
Builds on the work done in #402 .
Fixes #369
Fixes #573