Skip to content

Conversation

Xeom
Copy link

@Xeom Xeom commented Feb 9, 2024

Linux doesn't set the amaster parameter of forkpty() to a valid file descriptor in the child, but we pass this value to OwnedFd::from_raw_fd, which panics if the passed value is 0xffff_ffff.

A simple reproducer:

fn effify()
{
    let x: [u8; 1024] = [0xff; 1024];
}

fn main()
{
    effify();

    let dupped = nix::unistd::dup(1).unwrap();
    let result = unsafe { nix::pty::forkpty(None, None).unwrap() };

    match result.fork_result
    {
        nix::unistd::ForkResult::Child =>
        {
            nix::unistd::dup2(dupped, 1).unwrap();
            println!("Child is happy");
        }
        nix::unistd::ForkResult::Parent { child } =>
        {
            println!("Parent is happy");
            println!("{:?}", nix::sys::wait::waitpid(child, None).unwrap());
        }
    }
}

The child process panics in the above code without this patch applied.

What does this PR do

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

Linux does set the amaster parameter of `forkpty()` to a valid file
descriptor in the child, but we pass this value to
`OwnedFd::from_raw_fd`, which panics if the passed value is
`0xffff_ffff`.

A simple reproducer:

```
fn effify()
{
    let x: [u8; 1024] = [0xff; 1024];
}

fn main()
{
    effify();

    let dupped = nix::unistd::dup(1).unwrap();
    let result = unsafe { nix::pty::forkpty(None, None).unwrap() };

    match result.fork_result
    {
        nix::unistd::ForkResult::Child =>
        {
            nix::unistd::dup2(dupped, 1).unwrap();
            println!("Child is happy");
        }
        nix::unistd::ForkResult::Parent { child } =>
        {
            println!("Parent is happy");
            println!("{:?}", nix::sys::wait::waitpid(child, None).unwrap());
        }
    }
}
```

The child process panics in the above code without the above patch
applied.
@Xeom Xeom changed the title Fix undefined memory read and associated crash. Fix uninitialized memory read and associated crash. Feb 9, 2024
@Xeom
Copy link
Author

Xeom commented Feb 11, 2024

Apologies for the spelling mistakes in the commit message, I was sleepy when I wrote this.

Now I'm awake however, I've realized that this patch fixes my particular crash, but not the more general issue that we're putting an uninitialized value into an OwnedFd in the child process.

In my case, I immediately call exec() in my child process, so this doesn't matter for my use case, but if I didn't, then the OwnedFd in the ForkptyResult would be dropped in the child process, closing an uninitialized file descriptor. My patch would simply cause this file descriptor to always be stdin, which doesn't help too much.

A proper fix for this problem would be to change the structure of ForkptyResult so that master is present only in the parent process.

@SteveLauC
Copy link
Member

SteveLauC commented Feb 16, 2024

I can reproduce this on my Linux host (kernel 6.7.4/glibc 2.38). And from the source code of glibc 2.39, in the spawned child process, the amster buffer won't be accessed.

A proper fix for this problem would be to change the structure of ForkptyResult so that master is present only in the parent process.

This makes sense to me, at least on Linux/Glibc.

Unfortunately, the Linux manual does not mention this behavior. This syscall comes from BSD, the FreeBSD manual, does mention this:

In the child process, it closes the descriptor for the master side of the pty

So I think we can apply the patch to all the platforms.

@asomers
Copy link
Member

asomers commented Feb 25, 2024

superseded by #2315

@asomers asomers closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants