Skip to content

Commit

Permalink
container: Enable wrappers duing transaction
Browse files Browse the repository at this point in the history
This ensures that we get the same wrappers for `/usr/bin/systemctl`
and `/usr/bin/groupadd` in the container flow that we have
at `compose tree` time.

(Also, it's really confusing that this functionality is separate
 from `cliwrap`...we need to unify these two things)

While I'm here, I hit an exciting bug in the container flow where
I accidentally typo'd a package name, and because we didn't
"undo on drop" we left our wrappers active.  (This doesn't matter
on the compose side as we're always acting on a temporary filesystem)

Bigger picture we should do
https://internals.rust-lang.org/t/should-rust-programs-unwind-on-sigint/13800/11
  • Loading branch information
cgwalters committed Oct 6, 2022
1 parent fea26bd commit 3526bb5
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 16 deletions.
6 changes: 3 additions & 3 deletions rpmostree-cxxrs.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ struct TempEtcGuard final : public ::rust::Opaque
#define CXXBRIDGE1_STRUCT_rpmostreecxx$FilesystemScriptPrep
struct FilesystemScriptPrep final : public ::rust::Opaque
{
void undo () const;
void undo ();
~FilesystemScriptPrep () = delete;

private:
Expand Down Expand Up @@ -2069,7 +2069,7 @@ extern "C"
::std::int32_t rootfs, ::rust::Box< ::rpmostreecxx::FilesystemScriptPrep> *return$) noexcept;

::rust::repr::PtrLen rpmostreecxx$cxxbridge1$FilesystemScriptPrep$undo (
const ::rpmostreecxx::FilesystemScriptPrep &self) noexcept;
::rpmostreecxx::FilesystemScriptPrep &self) noexcept;

::rust::repr::PtrLen rpmostreecxx$cxxbridge1$run_depmod (::std::int32_t rootfs_dfd,
::rust::Str kver,
Expand Down Expand Up @@ -3719,7 +3719,7 @@ prepare_filesystem_script_prep (::std::int32_t rootfs)
}

void
FilesystemScriptPrep::undo () const
FilesystemScriptPrep::undo ()
{
::rust::repr::PtrLen error$ = rpmostreecxx$cxxbridge1$FilesystemScriptPrep$undo (*this);
if (error$.ptr)
Expand Down
2 changes: 1 addition & 1 deletion rpmostree-cxxrs.h
Original file line number Diff line number Diff line change
Expand Up @@ -1225,7 +1225,7 @@ struct TempEtcGuard final : public ::rust::Opaque
#define CXXBRIDGE1_STRUCT_rpmostreecxx$FilesystemScriptPrep
struct FilesystemScriptPrep final : public ::rust::Opaque
{
void undo () const;
void undo ();
~FilesystemScriptPrep () = delete;

private:
Expand Down
30 changes: 22 additions & 8 deletions rust/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ pub(crate) fn refspec_classify(refspec: &str) -> crate::ffi::RefspecType {
/// Perform reversible filesystem transformations necessary before we execute scripts.
pub(crate) struct FilesystemScriptPrep {
rootfs: Dir,
enabled: bool,
}

pub(crate) fn prepare_filesystem_script_prep(rootfs: i32) -> CxxResult<Box<FilesystemScriptPrep>> {
Expand Down Expand Up @@ -166,12 +167,18 @@ impl FilesystemScriptPrep {
rootfs.atomic_write_with_perms(path, contents, mode)?;
}
}
Ok(Box::new(Self { rootfs }))
Ok(Box::new(Self {
rootfs,
enabled: true,
}))
}

/// Undo the filesystem changes.
#[context("Undoing prep filesystem for scripts")]
pub(crate) fn undo(&self) -> CxxResult<()> {
pub(crate) fn undo(&mut self) -> CxxResult<()> {
if !self.enabled {
return Ok(());
}
for &path in Self::OPTIONAL_PATHS
.iter()
.chain(Self::REPLACE_OPTIONAL_PATHS.iter().map(|x| &x.0))
Expand All @@ -181,10 +188,17 @@ impl FilesystemScriptPrep {
self.rootfs.rename(saved, &self.rootfs, path)?;
}
}
self.enabled = false;
Ok(())
}
}

impl Drop for FilesystemScriptPrep {
fn drop(&mut self) {
let _ = self.undo();
}
}

/// Some Fedora/RHEL kernels ship .hmac files with absolute paths inside,
/// which breaks when we relocate them into ostree/. This function
/// changes them to be relative.
Expand Down Expand Up @@ -363,7 +377,7 @@ mod test {
let d = cap_tempfile::tempdir(cap_std::ambient_authority())?;
// The no-op case
{
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
g.undo()?;
}
let mut db = dirbuilder_from_mode(0o755);
Expand All @@ -375,7 +389,7 @@ mod test {
// Neutered sss_cache.
{
assert!(d.try_exists(super::SSS_CACHE_PATH)?);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
assert!(!d.try_exists(super::SSS_CACHE_PATH)?);
g.undo()?;
assert!(d.try_exists(super::SSS_CACHE_PATH)?);
Expand All @@ -386,7 +400,7 @@ mod test {
d.atomic_write_with_perms(super::SYSTEMCTL_PATH, original_systemctl, mode.clone())?;
let contents = d.read_to_string(super::SYSTEMCTL_PATH)?;
assert_eq!(contents, original_systemctl);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::SYSTEMCTL_PATH)?;
assert_eq!(contents.as_bytes(), super::SYSTEMCTL_WRAPPER);
g.undo()?;
Expand All @@ -399,7 +413,7 @@ mod test {
d.atomic_write_with_perms(super::GROUPADD_PATH, original_groupadd, mode.clone())?;
let contents = d.read_to_string(super::GROUPADD_PATH)?;
assert_eq!(contents, original_groupadd);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::GROUPADD_PATH)?;
assert_eq!(contents.as_bytes(), super::GROUPADD_WRAPPER);
g.undo()?;
Expand All @@ -412,7 +426,7 @@ mod test {
d.atomic_write_with_perms(super::USERADD_PATH, original_useradd, mode.clone())?;
let contents = d.read_to_string(super::USERADD_PATH)?;
assert_eq!(contents, original_useradd);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::USERADD_PATH)?;
assert_eq!(contents.as_bytes(), super::USERADD_WRAPPER);
g.undo()?;
Expand All @@ -425,7 +439,7 @@ mod test {
d.atomic_write_with_perms(super::USERMOD_PATH, original_usermod, mode)?;
let contents = d.read_to_string(super::USERMOD_PATH)?;
assert_eq!(contents, original_usermod);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let mut g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::USERMOD_PATH)?;
assert_eq!(contents.as_bytes(), super::USERMOD_WRAPPER);
g.undo()?;
Expand Down
2 changes: 1 addition & 1 deletion rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ pub mod ffi {
fn undo(self: &TempEtcGuard) -> Result<()>;

fn prepare_filesystem_script_prep(rootfs: i32) -> Result<Box<FilesystemScriptPrep>>;
fn undo(self: &FilesystemScriptPrep) -> Result<()>;
fn undo(self: &mut FilesystemScriptPrep) -> Result<()>;

fn run_depmod(rootfs_dfd: i32, kver: &str, unified_core: bool) -> Result<()>;

Expand Down
12 changes: 9 additions & 3 deletions src/libpriv/rpmostree-container.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ rpmostree_container_rebuild (rpmostreecxx::Treefile &treefile, GCancellable *can
g_autoptr (RpmOstreeContext) ctx = rpmostree_context_new_container ();
rpmostree_context_set_treefile (ctx, treefile);

glnx_autofd int rootfs_fd = -1;
if (!glnx_opendirat (AT_FDCWD, "/", TRUE, &rootfs_fd, error))
return FALSE;

// Ensure we have our wrappers for groupadd/systemctl set up
CXX_TRY_VAR (fs_prep, rpmostreecxx::prepare_filesystem_script_prep (rootfs_fd), error);

if (!rpmostree_context_setup (ctx, "/", "/", cancellable, error))
return FALSE;

Expand All @@ -61,9 +68,8 @@ rpmostree_container_rebuild (rpmostreecxx::Treefile &treefile, GCancellable *can
if (!dnf_context_run (dnfctx, NULL, error))
return FALSE;

glnx_autofd int rootfs_fd = -1;
if (!glnx_opendirat (AT_FDCWD, "/", TRUE, &rootfs_fd, error))
return FALSE;
CXX_TRY (fs_prep->undo (), error);

CXX_TRY (rpmostreecxx::postprocess_cleanup_rpmdb (rootfs_fd), error);

return TRUE;
Expand Down

0 comments on commit 3526bb5

Please sign in to comment.