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

Recent changes to posix spawn seems to cause a segfault in std::process::Command #3677

Open
antoyo opened this issue May 2, 2024 · 4 comments · May be fixed by #3710
Open

Recent changes to posix spawn seems to cause a segfault in std::process::Command #3677

antoyo opened this issue May 2, 2024 · 4 comments · May be fixed by #3710
Labels
C-bug Category: bug

Comments

@antoyo
Copy link

antoyo commented May 2, 2024

Hi.
The CI of rustc_codegen_gcc suddendly started failing.
After investigation, it seems to be caused by recent changes to libc, regarding posix spawn stuff (so possibly related to this change).
My target triple is: x86_64-unknown-linux-gnu.

Here's the code (inspired by std::process::Command) that works with libc 0.2.153, but segfaults with 0.2.154 (it doesn't reproduce in the playground since it still uses version 0.2.153):

use std::{ffi::OsStr, io};
use std::ffi::{c_int, c_ulong, c_short};

use libc::{gid_t, pid_t, uid_t};
//use std::os::unix::raw::{uid_t, gid_t, pid_t};

mod imp {
    use std::{ffi::{CString, OsStr}, io, os::unix::ffi::OsStrExt, process::Stdio};

    use libc::{gid_t, pid_t, uid_t};
    //use std::os::unix::raw::{uid_t, gid_t, pid_t};

    pub struct Command {
        pub program: CString,
        args: Vec<CString>,
        cwd: Option<CString>,
        pub uid: Option<uid_t>,
        pub gid: Option<gid_t>,
        saw_nul: bool,
        pub closures: Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>>,
        pub groups: Option<Box<[gid_t]>>,
        stdin: Option<Stdio>,
        stdout: Option<Stdio>,
        stderr: Option<Stdio>,
        #[cfg(target_os = "linux")]
        pub create_pidfd: bool,
        pub pgroup: Option<pid_t>,
    }

    fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
        CString::new(s.as_bytes()).unwrap_or_else(|_e| {
            *saw_nul = true;
            CString::new("<string-with-nul>").unwrap()
        })
    }

    impl Command {
        pub fn new(program: &OsStr) -> Command {
            let mut saw_nul = false;
            let program = os2c(program, &mut saw_nul);
            Command {
                args: vec![program.clone()],
                program,
                cwd: None,
                uid: None,
                gid: None,
                saw_nul,
                closures: Vec::new(),
                groups: None,
                stdin: None,
                stdout: None,
                stderr: None,
                create_pidfd: false,
                pgroup: None,
            }
        }
    }
}

pub fn cvt_nz(error: c_int) -> io::Result<()> {
    if error == 0 {
        return Ok(())
    }
    panic!();
}

pub struct Command {
    inner: imp::Command,
}

impl Command {
    pub fn new<S: AsRef<OsStr>>(program: S) -> Command {
        Command { inner: imp::Command::new(program.as_ref()) }
    }

    pub fn get_pgroup(&self) -> Option<pid_t> {
        self.inner.pgroup
    }

    pub fn get_create_pidfd(&self) -> bool {
        self.inner.create_pidfd
    }

    pub fn get_uid(&self) -> Option<uid_t> {
        self.inner.uid
    }

    pub fn get_gid(&self) -> Option<gid_t> {
        self.inner.gid
    }

    pub fn env_saw_path(&self) -> bool {
        false
    }

    pub fn program_is_path(&self) -> bool {
        self.inner.program.to_bytes().contains(&b'/')
    }

    pub fn get_closures(&mut self) -> &mut Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>> {
        &mut self.inner.closures
    }

    pub fn get_groups(&self) -> Option<&[gid_t]> {
        self.inner.groups.as_deref()
    }

    fn posix_spawn(
        &mut self,
    ) -> io::Result<Option<i32>> {
        use std::mem::MaybeUninit;

        if self.get_gid().is_some()
            || self.get_uid().is_some()
            || (self.env_saw_path() && !self.program_is_path())
            || !self.get_closures().is_empty()
            || self.get_groups().is_some()
            || self.get_create_pidfd()
        {
            return Ok(None);
        }

        #[repr(C)]
        struct sched_param {
            sched_priority: c_int,
        }

        #[repr(C)]
        struct sigset_t {
            __val: [c_ulong; 16],
        }

        #[repr(C)]
        pub struct posix_spawnattr_t {
            __flags: c_short,
            __pgrp: pid_t,
            __sd: sigset_t,
            __ss: sigset_t,
            #[cfg(any(target_env = "musl", target_env = "ohos"))]
            __prio: c_int,
            #[cfg(not(any(target_env = "musl", target_env = "ohos")))]
            __sp: sched_param,
            __policy: c_int,
            __pad: [c_int; 16],
        }

        extern "C" {
            fn posix_spawnattr_init(attr: *mut posix_spawnattr_t) -> c_int;
        }

        unsafe {
            let mut attrs = MaybeUninit::uninit();
            //cvt_nz(posix_spawnattr_init(attrs.as_mut_ptr()))?; // This works.
            cvt_nz(libc::posix_spawnattr_init(attrs.as_mut_ptr()))?; // This segfaults.
            Ok(None)
        }
    }
}

fn main() {
    let mut command = Command::new("ls");
    command.posix_spawn().unwrap();
}

It seems there's stack corruption.

It could be interesting to run the std tests in the CI to check if changes to libc breaks anything.

Thanks.

Edit: Pinning to the version 0.2.153 fixes the CI in rustc_codegen_gcc: rust-lang/rustc_codegen_gcc#512

@antoyo antoyo added the C-bug Category: bug label May 2, 2024
@JohnTitor
Copy link
Member

#3602 should be related (note that the changes to v0.2 exist on the libc-0.2 branch).
I'm going to revert that PR as a quick fix: #3678

@Colecf
Copy link

Colecf commented May 3, 2024

+1 we're also seeing segfaults after updating libc to 0.2.154 with code that uses posix_spawn directly.

@faptc
Copy link

faptc commented May 4, 2024

Edit: Same conclusion as in #3608

Linux posix_spawnattr_init really is just an wrapper for memset an value of type posix_spawnattr_t. #3602 changed posix_spawnattr_t to *mut c_void, and that caused a buffer overflow.

    unsafe {
        let mut attrs = <MaybeUninit<*mut c_void>>::uninit();
        let ptr = attrs.as_mut_ptr();
        memset(ptr as _, 0, size_of::<libc::posix_spawnattr_t>());
    }

On Android (with Bionic libc), posix_spawnattr_init calls an calloc that allocates
a new posix_spawnattr_t. Which is valid on that target.

I propose maintainers to yank libc v0.2.154.

@madsmtm
Copy link
Contributor

madsmtm commented May 17, 2024

#3690 has been merged, and 0.2.155 has been released, so I believe this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants