Skip to content

Cmd pdeathsig #943

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

Merged
merged 2 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,8 @@ async fn install_container(
/// Run a command in the host mount namespace
pub(crate) fn run_in_host_mountns(cmd: &str) -> Command {
let mut c = Command::new("/proc/self/exe");
c.args(["exec-in-host-mount-namespace", cmd]);
c.lifecycle_bind()
.args(["exec-in-host-mount-namespace", cmd]);
c
}

Expand Down
12 changes: 5 additions & 7 deletions lib/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::process::Command;
use std::time::Duration;

use anyhow::{Context, Result};
use bootc_utils::CommandRunExt;
#[cfg(feature = "install")]
use camino::Utf8Path;
use cap_std_ext::cap_std::fs::Dir;
Expand Down Expand Up @@ -116,15 +117,12 @@ pub(crate) fn spawn_editor(tmpf: &tempfile::NamedTempFile) -> Result<()> {
let argv0 = editor_args
.next()
.ok_or_else(|| anyhow::anyhow!("Invalid editor: {editor}"))?;
let status = Command::new(argv0)
Command::new(argv0)
.args(editor_args)
.arg(tmpf.path())
.status()
.context("Spawning editor")?;
if !status.success() {
anyhow::bail!("Invoking editor: {editor} failed: {status:?}");
}
Ok(())
.lifecycle_bind()
.run()
.with_context(|| format!("Invoking editor {editor} failed"))
}

/// Convert a combination of values (likely from CLI parsing) into a signature source
Expand Down
1 change: 1 addition & 0 deletions ostree-ext/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ ostree = { features = ["v2022_6"], version = "0.19.0" }

# Private dependencies
anyhow = { workspace = true }
bootc-utils = { path = "../utils" }
camino = { workspace = true, features = ["serde1"] }
chrono = { workspace = true }
olpc-cjson = "0.1.1"
Expand Down
2 changes: 2 additions & 0 deletions ostree-ext/src/container/deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::os::fd::BorrowedFd;
use std::process::Command;

use anyhow::Result;
use bootc_utils::CommandRunExt;
use cap_std_ext::cmdext::CapStdExtCommandExt;
use fn_error_context::context;
use ocidir::cap_std::fs::Dir;
Expand Down Expand Up @@ -148,6 +149,7 @@ pub async fn deploy(
let st = Command::new("/proc/self/exe")
.args(["internals", "bootc-install-completion", ".", stateroot])
.cwd_dir(sysroot_dir.try_clone()?)
.lifecycle_bind()
.status()?;
if !st.success() {
anyhow::bail!("Failed to complete bootc install");
Expand Down
1 change: 1 addition & 0 deletions utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ repository = "https://github.com/containers/bootc"

[dependencies]
anyhow = { workspace = true }
rustix = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
tempfile = { workspace = true }
Expand Down
17 changes: 17 additions & 0 deletions utils/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::{
io::{Read, Seek},
os::unix::process::CommandExt,
process::Command,
};

Expand All @@ -15,6 +16,9 @@ pub trait CommandRunExt {
/// Execute the child process.
fn run(&mut self) -> Result<()>;

/// Ensure the child does not outlive the parent.
fn lifecycle_bind(&mut self) -> &mut Self;

/// Execute the child process and capture its output. This uses `run` internally
/// and will return an error if the child process exits abnormally.
fn run_get_output(&mut self) -> Result<Box<dyn std::io::BufRead>>;
Expand Down Expand Up @@ -84,6 +88,19 @@ impl CommandRunExt for Command {
self.status()?.check_status(stderr)
}

#[allow(unsafe_code)]
fn lifecycle_bind(&mut self) -> &mut Self {
// SAFETY: This API is safe to call in a forked child.
Copy link
Contributor

Choose a reason for hiding this comment

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

A docs reference would be good (even if it's just to say that this is implemented with prctl and point to the list in signal-safety manpage)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, the rustix docs for this function do link directly to the prctl man page right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think while we could have more safety explanations in cases like this, what we have here is actually better than some of the other (few) unsafe cases and if we were to try to raise the bar we'd need to a bit more

unsafe {
self.pre_exec(|| {
rustix::process::set_parent_process_death_signal(Some(
rustix::process::Signal::Term,
))
.map_err(Into::into)
})
}
}

/// Output a debug-level log message with this command.
fn log_debug(&mut self) -> &mut Self {
// We unconditionally log at trace level, so avoid double logging
Expand Down
Loading