-
Notifications
You must be signed in to change notification settings - Fork 654
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
Fix special ptraces #686
Fix special ptraces #686
Conversation
The boxing and unboxing was unnecessary and instead the references to the type on the stack can just be cast.
@xd009642 I'd appreciate if you could take a look over this and maybe test it out as well. |
Just did a simple test and was able to get data and read it! All seems to be good The test was very hacky but easy to clean up, if you'd be interested in reimplementing it nicely? Essentially, I forked the test process, had the child make a PTRACE_TRACEME request and from the parent used waitpid to find when the child stopped and called ptrace_get_data<user_regs_struct> with a PTRACE_GETREGS request. Since the data came back okay I know I can read the x86_64 registers for the child process. It's a fairly simple addition and should stop similar issues with ptrace_get_data slipping through the net again. Would also be worth adding a ptrace_getregs as a nice interface to get registers. Or if that's too many changes and you'd rather get it merged in sooner I can open a pull request with those additions 👍 |
Awesome, thanks for checking this out @xd009642!
We'd like to avoid forking if possible in our test code because the Rust test harness can deadlock if you do that. It's been a huge source of issues with our CI and @asomers has been working diligently to remove them. Any way this can be tested without
This is in-progress in #666 so I'd suggest you hop in there and read through the discussion. It's still in the planning stages, but they'd of course appreciate any feedback! |
The easiest way I can see for avoiding fork is making a small separate project that gets launched by the test. You need another process to trace and that process to emit a SIGSTOP at some point. Apart from that creating a new thread, you may be able to ptrace the thread id, but I think that works because new threads on linux are typically caused by a fork of some form (afaik). It is a tricky area to test because of the reliance on another processes. Maybe the solution is for a cargo project to enable testing of these features? Launch it as a process with different flags to enable different behaviours. There might be a way to create something like that which doesn't have the same problems fork has. |
Yeah, I think this is something that we might need to avoid the test harness completely (like we currently do for mount) and do that. But that wouldn't be too bad. Can you raise a new issue and post your code snippet? I don't want to hold up this PR for this, but I'd like to get it written down somewhere. bors r+ xd009642 |
686: Fix special ptraces r=Susurrus In #614 we added specializations of `ptrace()` that added more type safety. As part of this, the `UnsupportedOperation` error was introduced for the requests that are covered by specialized versions so they couldn't be used with the general `ptrace()`. Unfortunately, no tests were added with this PR and so it slipped through that you could not do those operations at all anymore: `ptrace()` reported `UnsupportedOperation` for them and `ptrace_*` called `ptrace`, not `ffi::ptrace` and so also reported `UnsupportedOperation`! Whoops! This minimally-invasive surgery corrects this by adding tests that call all the specialized `ptrace_*` ignoring the return value save checking for `UnsupportedOperation`. It also changes the functions calls to use `ffi::ptrace()` directly to fix the bug. As this was never a bug in a released version of `nix`, there's no need for a changelog entry here.
@xd009642 I can buy that testing ptrace is inevitably going to require multiple processes. I can suggest three ways to do it, and you can pick the easiest:
|
My little hacky test is immortalised in issue #687. I'll have a look at a testing program and raise another pr at some point (hopefully soon). If on the issue you could comment other areas of nix which could stand to be tested via the same program it'll let me spot any potential bonus tests I could bundle in (if it's easy enough) |
Build failed |
bors r+ xd009642 |
686: Fix special ptraces r=Susurrus In #614 we added specializations of `ptrace()` that added more type safety. As part of this, the `UnsupportedOperation` error was introduced for the requests that are covered by specialized versions so they couldn't be used with the general `ptrace()`. Unfortunately, no tests were added with this PR and so it slipped through that you could not do those operations at all anymore: `ptrace()` reported `UnsupportedOperation` for them and `ptrace_*` called `ptrace`, not `ffi::ptrace` and so also reported `UnsupportedOperation`! Whoops! This minimally-invasive surgery corrects this by adding tests that call all the specialized `ptrace_*` ignoring the return value save checking for `UnsupportedOperation`. It also changes the functions calls to use `ffi::ptrace()` directly to fix the bug. As this was never a bug in a released version of `nix`, there's no need for a changelog entry here.
Build failed |
bors r+ xd009642 |
686: Fix special ptraces r=Susurrus In #614 we added specializations of `ptrace()` that added more type safety. As part of this, the `UnsupportedOperation` error was introduced for the requests that are covered by specialized versions so they couldn't be used with the general `ptrace()`. Unfortunately, no tests were added with this PR and so it slipped through that you could not do those operations at all anymore: `ptrace()` reported `UnsupportedOperation` for them and `ptrace_*` called `ptrace`, not `ffi::ptrace` and so also reported `UnsupportedOperation`! Whoops! This minimally-invasive surgery corrects this by adding tests that call all the specialized `ptrace_*` ignoring the return value save checking for `UnsupportedOperation`. It also changes the functions calls to use `ffi::ptrace()` directly to fix the bug. As this was never a bug in a released version of `nix`, there's no need for a changelog entry here.
Build succeeded |
In #614 we added specializations of
ptrace()
that added more type safety. As part of this, theUnsupportedOperation
error was introduced for the requests that are covered by specialized versions so they couldn't be used with the generalptrace()
. Unfortunately, no tests were added with this PR and so it slipped through that you could not do those operations at all anymore:ptrace()
reportedUnsupportedOperation
for them andptrace_*
calledptrace
, notffi::ptrace
and so also reportedUnsupportedOperation
! Whoops!This minimally-invasive surgery corrects this by adding tests that call all the specialized
ptrace_*
ignoring the return value save checking forUnsupportedOperation
. It also changes the functions calls to useffi::ptrace()
directly to fix the bug.As this was never a bug in a released version of
nix
, there's no need for a changelog entry here.