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

os::unix::process::Command::exec sometimes allocates, violating async signal safety #130756

Open
colinmarc opened this issue Sep 23, 2024 · 6 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@colinmarc
Copy link

I'm trying to build a small linux container runtime as part of another project. I'd like to do the moral equivalent of the following (extracted out and untested):

fn spawn_in_container(cmd: std::process::Command) -> anyhow::Result<u32> {
    let mut args = clone3::Clone3::default();
    args.exit_signal(libc::SIGCHLD as _)
        .flag_newuser()
        .flag_newns()
        .flag_newpid();

    match unsafe { args.call().context("clone3")? } {
        0 => unsafe { self.child_after_fork(cmd) },
        pid => return Ok(pid),
    }
}

// SAFETY: blah blah blah we can't allocate or anything else
unsafe fn child_after_fork(cmd: std::process::Command) -> ! {
    // ... various container setup

    // If successful, this never returns.
    let e = cmd.exec();
    std::process::abort();
}

do_exec in process_unix.rs makes a big deal about the (un)safety of this operation, so I thought that it would be safe to use Command::exec. Unfortunately, I just caught a deadlock:

#0  0x000072ca4efb0c0b in __lll_lock_wait_private () from target:/usr/lib/libc.so.6
#1  0x000072ca4efc5138 in malloc () from target:/usr/lib/libc.so.6
#2  0x00006458e3b79d7f in alloc::alloc::alloc () at library/alloc/src/alloc.rs:100
#3  alloc::alloc::Global::alloc_impl () at library/alloc/src/alloc.rs:183
#4  alloc::alloc::{impl#1}::allocate () at library/alloc/src/alloc.rs:243
#5  alloc::raw_vec::RawVec::try_allocate_in<u8, alloc::alloc::Global> () at library/alloc/src/raw_vec.rs:230
#6  alloc::raw_vec::RawVec::with_capacity_in<u8, alloc::alloc::Global> () at library/alloc/src/raw_vec.rs:158
#7  alloc::vec::Vec::with_capacity_in<u8, alloc::alloc::Global> () at library/alloc/src/vec/mod.rs:699
#8  alloc::slice::hack::{impl#1}::to_vec<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:162
#9  alloc::slice::hack::to_vec<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:111
#10 alloc::slice::{impl#0}::to_vec_in<u8, alloc::alloc::Global> () at library/alloc/src/slice.rs:478
#11 alloc::vec::{impl#11}::clone<u8, alloc::alloc::Global> () at library/alloc/src/vec/mod.rs:2843
#12 std::sys::os_str::bytes::{impl#4}::clone () at library/std/src/sys/os_str/bytes.rs:73
#13 std::ffi::os_str::{impl#10}::clone () at library/std/src/ffi/os_str.rs:641
#14 std::sys_common::process::CommandEnv::capture () at library/std/src/sys_common/process.rs:45
#15 std::sys_common::process::CommandEnv::capture_if_changed () at library/std/src/sys_common/process.rs:58
#16 std::sys::pal::unix::process::process_common::Command::capture_env () at library/std/src/sys/pal/unix/process/process_common.rs:363
#17 0x00006458e3b71913 in std::sys::pal::unix::process::process_common::Command::exec () at library/std/src/sys/pal/unix/process/process_unix.rs:237
#18 std::os::unix::process::{impl#0}::exec () at library/std/src/os/unix/process.rs:227

Something in capture_env is allocating, which violates the rules around what you're allowed to do between fork or clone and exec.

As far as I can tell, this isn't documented one way or the other. So maybe this is a documentation bug, or I missed the documentation. Still, the amount of surface area that has the potential to allocate seems very small here - maybe the allocation would be possible to avoid? That would let me and others use the stdlib Command for this use-case, which would be pretty nice.

Meta

rustc --version --verbose:

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7
@colinmarc colinmarc added the C-bug Category: This is a bug. label Sep 23, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 23, 2024
@colinmarc
Copy link
Author

colinmarc commented Sep 23, 2024

I just found the following note:

fn exec(&mut self) -> io::Error {
// NOTE: This may *not* be safe to call after `libc::fork`, because it
// may allocate. That may be worth fixing at some point in the future.
self.as_inner_mut().exec(sys::process::Stdio::Inherit)
}

Needless to say, my vote is that it's worth fixing :)

@jieyouxu jieyouxu added T-libs Relevant to the library team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Sep 23, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 23, 2024
@the8472
Copy link
Member

the8472 commented Sep 23, 2024

let mut args = clone3::Clone3::default();

note: according to libc authors it's not possible to use the standard library (or anything calling into libc) after using clone3
#89522 (comment) #89522 (comment)

@colinmarc
Copy link
Author

colinmarc commented Sep 23, 2024

note: according to libc authors it's not possible to use the standard library (or anything calling into libc) after using clone3 #89522 (comment) #89522 (comment)

Thanks - that's really good to know. I'm probably okay using libc clone fork and doing something like #113939.

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 24, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 25, 2024
colinmarc added a commit to colinmarc/magic-mirror that referenced this issue Oct 15, 2024
colinmarc added a commit to colinmarc/magic-mirror that referenced this issue Oct 21, 2024
colinmarc added a commit to colinmarc/magic-mirror that referenced this issue Oct 21, 2024
@the8472
Copy link
Member

the8472 commented Nov 25, 2024

This is not an I-unsound issue in std because Command::exec does not make such a guarantee. The onus is on the users of fork or pre_exec to ensure that the requirements are upheld. This either requires only forking a single-threaded program or dropping down libc functions directly.

If you want Command::exec to guarantee the absence of allocations that would be an API change request

@the8472 the8472 added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels Nov 25, 2024
@the8472
Copy link
Member

the8472 commented Nov 25, 2024

Thinking about it, changing the API would be difficult. Preparing a new environment is part of Command's purpose and documented behavior of exec, which requires allocation. And it cannot be done it advance because the environment might change between the time command is constructed and the time exec gets called.
So updating the documentation is probably the best approach.

@colinmarc
Copy link
Author

Thinking about it, changing the API would be difficult. Preparing a new environment is part of Command's purpose and documented behavior of exec, which requires allocation.

Sorry if I misunderstood, but why does it necessarily require heap allocation? Note that to be safe in this sense, it doesn't have to avoid allocations during setup - just during the call to exec.

If you want Command::exec to guarantee the absence of allocations that would be an API change request

Isn't this just underspecified? A perfectly reasonable interpretation of the current docs is that it is careful about allocations, given the callout to execvp and all the fuss made about allocations in the documentation to pre_exec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants