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

Call to waitpid with pid of -1 can panic #776

Closed
xd009642 opened this issue Oct 6, 2017 · 4 comments
Closed

Call to waitpid with pid of -1 can panic #776

xd009642 opened this issue Oct 6, 2017 · 4 comments
Labels

Comments

@xd009642
Copy link
Contributor

xd009642 commented Oct 6, 2017

When calling waitpid(-1, SOME(__WALL)) the following line in src/sys/wait.rs can cause a panic.

fn stop_signal(status: i32) -> Signal {
    Signal::from_c_int(unsafe { libc::WSTOPSIG(status) }).unwrap()
}

I'm aware I'm on an old version of nix, and it is on my task list to update. However, this code exists in the latest version as well. Can this panic still happen? And is it something that should be fixed or down to the application author to ensure that such a situation won't occur?

In my case I have a feeling I might be misusing a pid slightly, so it could be fully user error and may or may not be an actual issue. But it's best to flag these things in case it is actually an issue.

Below is my stack trace (the nix section), once again, it's an older version, but it may still have some use!

  11: nix::sys::wait::status::stop_signal
             at .cargo/registry/src/git.luolix.top-1ecc6299db9ec823/nix-0.8.1/src/sys/wait.rs:84
  12: nix::sys::wait::decode::decode_stopped
             at .cargo/registry/src/git.luolix.top-1ecc6299db9ec823/nix-0.8.1/src/sys/wait.rs:199
  13: nix::sys::wait::decode
             at .cargo/registry/src/git.luolix.top-1ecc6299db9ec823/nix-0.8.1/src/sys/wait.rs:210
  14: nix::sys::wait::waitpid
             at .cargo/registry/src/git.luolix.top-1ecc6299db9ec823/nix-0.8.1/src/sys/wait.rs:231
@Susurrus
Copy link
Contributor

I'm not too familiar with all the signal stuff in POSIX and the nix wrappers around that. From what you say I can't debug what's going on because I don't know the intermediate values.

I'm wondering if we couldn't get a solid test to run that fails which would let us validate any possible fix for this in the future.

@Susurrus Susurrus added the A-bug label Jan 20, 2018
@Susurrus
Copy link
Contributor

@xd009642 What platform are you on, as the code is a little different from Android/Linux than other platforms? Additionally have you dug into debugging this anymore at all? It'd be really great to get a reduced test case into our test suite so we can be assured we've fixed the issue once we do.

Part of the problem here is that there are 4 different types of calls to waitpid():

  • wait for any child process with a specific process group ID
  • wait for any child process
  • wait for any child process whose process group ID is equal to that of the calling process
  • wait for the child with a specific process ID

So rather than use an int with ranges pertaining to those, we should use an enum type to clarify this.

@xd009642
Copy link
Contributor Author

Sorry I forgot all about this issue! I'm on Linux running Fedora. I think I was probably misusing PIDs and continuing or stopping the parent PID instead of the one which issued the stop as I was working on ptrace stop/step on multi-threaded programs.

So assuming I was misusing waitpid, I guess this raises the question of whether there should be unwraps in nix? Especially if they can cause panics, or whether this should be replaced with a Result type?

@Susurrus
Copy link
Contributor

Well glad you got it sorted out. Part of the problem is that you weren't on a recent version of nix, as you alluded to from the start. #741 actually addressed this issue for this piece of code. So I'd suggest that you update all of your projects to use the latest nix release, as we do get these things fixed regularly.

I'm going to close this issue, but I'll open a new one about revising the API for waitpid() as I suggested above.

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

No branches or pull requests

2 participants