Added ptrace utilities. #614

xd009642 · 2017-06-01T20:37:53Z

Some ptrace functions return structures through the data argument. This commit adds utilities to return data through this mechanism and function specialisations for a few of these functions (getting event messages or the siginfo_t struct). Once the next version of libc is released these utilities will be expanded to include the fpregs and user_regs structs.

Due to the need to use unsafe blocks to utilise these ptrace features I feel these are good candidates for inclusion into rust-nix

Susurrus · 2017-06-01T21:40:50Z

src/sys/ptrace.rs

@@ -91,6 +92,18 @@ fn ptrace_peek(request: ptrace::PtraceRequest, pid: pid_t, addr: *mut c_void, da
    }
 }

+/// Function for ptrace requests that return values from the data field.
+/// Not all structs that can currently be gotten via ptrace are implemented in


"be returned" instead of "be gotten"

Susurrus · 2017-06-01T21:44:27Z

How many different specialization of these functions do you expect? Is it a relatively small, finite number? If so, I'd rather keep ptrace_get_data private from the start and add a function for each specific use case. Otherwise keeping that public is a breaking API change when we go to set it private.

Also, is this available across all platforms?

xd009642 · 2017-06-01T21:51:04Z

I think it's only 2 or 3 more. But until the next version of libc comes out I can't implement them. I see what you mean with the breaking change and I'll move it to private as that's likely the best approach.

Some of them work across all platforms, looking at the ptrace documentation some won't work on SPARC systems like Solaris. However, the cfg in ptrace.rs limits it to x86, x86_64 and arm and I don't see a reason in the docs why it won't work on all of them.

xd009642 · 2017-06-01T21:53:06Z

Actually looking at the ptrace documentation I think it's more like 4 specialisations. Either way it is a finite number.

Susurrus

Mostly minor nitpicks, primarily around documentation. Please be more verbose and descriptive as it'll make reviewers happier! 😄

Susurrus · 2017-06-01T22:08:51Z

src/sys/ptrace.rs

+    ptrace_get_data::<c_long>(PTRACE_GETEVENTMSG, pid)
+}
+
+pub fn ptrace_getsiginfo(pid: pid_t) -> Result<siginfo_t> {


Needs a doccomment. Also linking to the manpages describing this functionality would be a good idea.

Susurrus · 2017-06-01T22:10:01Z

src/sys/ptrace.rs

+    ptrace_get_data::<siginfo_t>(PTRACE_GETSIGINFO, pid)
+}
+
+pub fn ptrace_setsiginfo(pid: pid_t, mut sig: siginfo_t) -> Result<c_long> {


Needs a doccomment. Also linking to the manpages describing this functionality would be a good idea.

Susurrus · 2017-06-01T22:10:45Z

src/sys/ptrace.rs

@@ -102,3 +112,19 @@ pub fn ptrace_setoptions(pid: pid_t, options: ptrace::PtraceOptions) -> Result<(

    ptrace(PTRACE_SETOPTIONS, pid, ptr::null_mut(), options as *mut c_void).map(drop)
 }
+
+/// Get the details of a ptrace event.


Can you be more descriptive than this? Also please link to a manpage here.

Susurrus · 2017-06-01T22:11:08Z

CHANGELOG.md

@@ -7,13 +7,15 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ### Added
 - Added `sys::signal::SigAction::{ flags, mask, handler}`
-  ([#611](https://github.com/nix-rust/nix/pull/609)
+  ([#611](https://github.com/nix-rust/nix/pull/609))


Please put these syntax fixes in a separate commit.

Susurrus · 2017-06-01T22:13:36Z

src/sys/ptrace.rs

+
+/// Get the details of a ptrace event.
+pub fn ptrace_getevent(pid: pid_t) -> Result<c_long> {
+    use self::ptrace::*;


This should be unnecessary.

Susurrus · 2017-06-01T22:14:23Z

src/sys/ptrace.rs

@@ -91,6 +92,15 @@ fn ptrace_peek(request: ptrace::PtraceRequest, pid: pid_t, addr: *mut c_void, da
    }
 }

+/// Function for ptrace requests that return values from the data field.
+fn ptrace_get_data<T>(request: ptrace::PtraceRequest, pid: pid_t) -> Result<T> {
+    let data: Box<T> = Box::new(unsafe { mem::uninitialized() });


This seems pretty complicated. Please improve the doccomment and also add inline comments describing why it's doing things. It's rather hard to parse right now.

Susurrus · 2017-06-02T18:23:10Z

src/sys/ptrace.rs

@@ -76,6 +77,7 @@ pub fn ptrace(request: ptrace::PtraceRequest, pid: pid_t, addr: *mut c_void, dat

    match request {
        PTRACE_PEEKTEXT | PTRACE_PEEKDATA | PTRACE_PEEKUSER => ptrace_peek(request, pid, addr, data),
+        PTRACE_GETEVENTMSG => ptrace_get_data::<c_long>(request, pid),


Why isn't this a standalone function like the others you had?

It was originally but I've of the review comments was that it seemed an unnecessary function. Unless I misunderstood?

No you didn't, but I think I was in the wrong there. I'd like to move towards all ptrace calls being implemented as dedicated functions. I'm wondering if we should also enforce that for the ones we make by making ptrace() return an error if you try to use GETEVENTMSG (or any of the other ones ) with it to get people to switch.

Susurrus · 2017-06-02T18:23:47Z

src/sys/ptrace.rs

+    // This creates a pointer to type T which is uninitialised. This is because 
+    // not all types may implement a default struct and some may have private members.
+    let data: Box<T> = Box::new(unsafe { mem::uninitialized() });
+    // Convert the pointer to store a result into a void ptr to input into ptrace


This is a useless comment.

xd009642 · 2017-06-02T21:09:22Z

So I'm working on some more tweaks based on comments. Regarding the suggestion to return errors when a user uses the ptrace function versus a specialisation. I'm not sure if that's the best approach given it may confuse users between actual ptrace errors and misuse of nix.

Also, the ptrace API is complicated and not oft used so documentation and examples are lacking. I wouldn't be fully confident that the specialisations are the only valid ways to use those ptrace requests and there aren't extended features for any of them available on other systems.

Susurrus · 2017-06-02T23:29:05Z

So I'm working on some more tweaks based on comments. Regarding the suggestion to return errors when a user uses the ptrace function versus a specialisation. I'm not sure if that's the best approach given it may confuse users between actual ptrace errors and misuse of nix.

The error would be something like UnsupportedOperation. It'd be pretty obvious as it'd also be documented.

Also, the ptrace API is complicated and not oft used so documentation and examples are lacking. I wouldn't be fully confident that the specialisations are the only valid ways to use those ptrace requests and there aren't extended features for any of them available on other systems.

Addressing this and the above, the goal of nix is to provide safe abstractions over the conventional POSIX APIs. That doesn't mean that everything POSIX should be in nix or vice versa as I see. Everything that can be made safe (as in provides a safer and/or Rusty-er API than the libc crate) should be made safe. For people who don't want the safety, like if they need more functionality, they can use the libc crate directly as they're not our target market.

xd009642 · 2017-06-03T16:28:22Z

That makes sense. Made the changes in question including the unsupported operation error return. I did have a look through the errno constants and that does seem to be the best fitting error.

Susurrus · 2017-06-03T17:11:27Z

I was actually thinking about specifying a new enum variant UnsupportedOperation within Error and this would return that. If you use Error::Sys(UnsupportedOperation) it might not be clear to our users why that error happened since that's an OS error but the OS actually supports that operation. We instead need a nix-specific error and report that instead. I'm also wondering if this variant should have a description so that we can point to an issue on GitHub or another function.

xd009642 · 2017-06-03T21:22:52Z

Adding an Error::Unsupported operation does make more sense. It is a bigger change though so I'd just like to clear a few things up before I push another commit.

When resolving to a errno (Error::errno), I take it UnknownErrno would be the preferred choice
When converting to an io::Error which ErrorKind is preferred? My first thought is ErrorKind::InvalidInput

Susurrus · 2017-06-04T22:56:29Z

When resolving to a errno (Error::errno), I take it UnknownErrno would be the preferred choice

When converting to an io::Error which ErrorKind is preferred? My first thought is ErrorKind::InvalidInput.

For both of these I don't think there should be a mapping. Mapping from Error to any of the other Error types that are in essence "subsets" shouldn't be necessary. I think the impl From<Error> for io::Error can just be deleted and I think Error::errno() should be deleted as well.

xd009642 · 2017-06-05T17:33:53Z

I've made the suggested changes, hopefully this clears up all the issues :)

Susurrus · 2017-06-07T06:30:25Z

src/lib.rs

@@ -97,6 +96,7 @@ pub enum Error {
    /// The operation involved a conversion to Rust's native String type, which failed because the
    /// string did not contain all valid UTF-8.
    InvalidUtf8,
+    UnsupportedOperation,


This needs a doccomment. It should make it clear that this isn't a libc error, but a nix-specific error indicating that the operation is not supported.

Susurrus · 2017-06-07T06:30:58Z

src/lib.rs

@@ -116,14 +116,6 @@ impl Error {
        Error::Sys(errno::EINVAL)
    }

-    /// Get the errno associated with this error


This is a public API, so removing this needs to be mentioned in the CHANGELOG under the Removed section.

Susurrus · 2017-06-07T06:31:20Z

src/lib.rs

            &Error::Sys(errno) => write!(f, "{:?}: {}", errno, errno.desc()),
        }
    }
 }

-impl From<Error> for io::Error {


Another public API removed, please list this in the Removed section of the CHANGELOG.

xd009642 · 2017-06-10T13:29:54Z

Hi, updated the changelog and the doc comments, sorry about the delay!

Susurrus · 2017-06-11T20:42:30Z

src/sys/ptrace.rs

+}
+
+/// Set siginfo as with `ptrace(PTRACE_SETSIGINFO,...)`
+pub fn ptrace_setsiginfo(pid: pid_t, mut sig: siginfo_t) -> Result<c_long> {


Shouldn't sig be &mut instead?

Susurrus · 2017-06-11T20:43:42Z

Just one last comment to address. Could you also rebase this on top of the latest HEAD and also squash all commits into 1?

xd009642 · 2017-06-11T21:30:19Z

Done 👍

Susurrus · 2017-06-11T23:25:24Z

Could you also address my comment about the argument type for ptrace_setsiginfo()?

xd009642 · 2017-06-12T16:29:47Z

Ah sorry missed that completely! I've made the fix and squashed the commits again so it's only one

Susurrus · 2017-06-12T18:37:25Z

Reading the docs a bit more it seems like ptrace_setsiginfo() should not actually have a mutable argument. The docs say it will "copy a siginfo_t structure from the address data in the tracer to the tracee", so I don't think it will modify that structure at all.

Looking at the possible error modes for these, it seems they will always return a valid system error, yes? So I think both ptrace_getevent() and ptrace_setsiginfo() should returnd Result<()> as the c_long is really an error value. That should be properly interpreted within those functions. See ptrace_setoptions() for an example.

xd009642 · 2017-06-12T19:31:45Z

ptrace_getevent() returns the event value which is specific to the event. For the fork/clone etc events it's the PID or TID of the new process for the exit event it's the exit code. The event type is determined by what waitpid returns (the nix-rust implementation of waitpid is great cause it lets you match to the events you're interested in).

I agree with you for ptrace_setsiginfo matching ptrace_setoptions but the c_long in getevent is important.

ptrace_setsiginfo's arguments are getting reverted and I'm making changes to the return type. I don't think ptrace_getevent needs changing at all so I'll leave that for now.

Susurrus · 2017-06-12T19:49:49Z

With ptrace_setsiginfo() I don't see why it needs the sig argument to be mut. It's definitely not needed for this specific event. And ptrace() takes a * const there anyways.

xd009642 · 2017-06-12T19:56:16Z

pub fn ptrace(request: ptrace::PtraceRequest, pid: pid_t, addr: *mut c_void, data: *mut c_void) -> Result<c_long>

I believe it's calling the public ptrace function we expose. I tried with const and without mut and I only got compiler errors. I could directly call the ptrace defined in the ffi namespace and that will likely solve it so I'll try that

Susurrus · 2017-06-12T19:59:11Z

Yeah, call ffi::ptrace() directly instead. I'd like to get that argument immutable. Then I think that's the last of this PR!

xd009642 · 2017-06-12T20:01:27Z

I was about to do that but then I realised that it was inconsistent with ptrace_setoptions. I can change ptrace_setoptions to make it consistent or might it be easier to just have it mutable since it's only in local scope and doesn't affect the caller (correct me if I'm wrong).

Susurrus · 2017-06-12T20:04:29Z

I'm okay with those being inconsistent at the implementation level. At least for consumers of this API, they won't be surprised by mut parameters.

Susurrus · 2017-06-12T20:05:11Z

The whole ffi module should go away anyways, in favor of this being implemented in libc. The whole ptrace thing needs some serious work done to it, so we'll defer that cleanup to that work.

Susurrus · 2017-06-12T20:44:04Z

src/sys/ptrace.rs

+}
+
+/// Set siginfo as with `ptrace(PTRACE_SETSIGINFO,...)`
+pub fn ptrace_setsiginfo(pid: pid_t, sig: siginfo_t) -> Result<()> {


There's no reason to take sig by-value since the values are copied out of the struct by the system. Let's change ptrace_setsiginfo to take it by-reference instead.

Susurrus · 2017-06-12T20:45:05Z

In addition to changing that variable to be by-reference, is there any way to set up tests for this? Since setsiginfo and getsiginfo are corollaries of each other it'd be nice if their APIs matched up nicely, which a test could easily verify (and that the code works as expected).

xd009642 · 2017-06-12T21:00:15Z

I'd have to look up more about the signal API in nix than I already know as I can't guarantee that writing certain signals won't affect the internal state of ptrace and cause error calls on some supported systems.

I have just ended up in a utter faff with git cause something messed up with my squashes and I ended up detaching my head so personally I'd prefer to close this once everything checks out. Then when the next version of libc is released and the user_regs struct is exposed I can implement the get and set regs ptrace function and do the tests for that and the get and set sig at the same time since there's potential for reuse there.

Susurrus · 2017-06-12T21:04:34Z

That's fair. Once this pasts tests then let's merge this! Except I don't see our FreeBSD buildbot running on these PRs. @asomers Can you look into this?

xd009642 · 2017-06-13T17:11:44Z

It looks like more PRs have been merged in the meantime. Should I bother rebasing now or wait until the freebsd issue is identified and we can be sure that we're ready to merge?

Some ptrace functions return structures through the data argument. This commit adds utilities to return data through this mechanism and function specialisations for a few of these functions (getting event messages or the siginfo_t struct). Once the next version of libc is released these utilities will be expanded to include the fpregs and user_regs structs. Ptrace requests that are now satisfied by a more specific public function will return an unsupported operation error. This has involved adding an UnsupportedOperation to the nix::Error enum and removed the mapping from Error to Errno and from Error to std::io::Error.

xd009642 · 2017-06-13T17:19:13Z

I decided since it was only quick I'd do it anyway.

Susurrus · 2017-06-13T18:57:23Z

I think the consensus is that if i686 freeBSD is working, then we'll keep plowing ahead. @asomers will be looking at it in the meantime.

Susurrus · 2017-06-13T18:58:02Z

bors r+

614: Added ptrace utilities. r=Susurrus Some ptrace functions return structures through the data argument. This commit adds utilities to return data through this mechanism and function specialisations for a few of these functions (getting event messages or the siginfo_t struct). Once the next version of libc is released these utilities will be expanded to include the fpregs and user_regs structs. Due to the need to use unsafe blocks to utilise these ptrace features I feel these are good candidates for inclusion into rust-nix

bors · 2017-06-13T21:55:05Z

Build succeeded

geofft · 2017-06-20T04:42:28Z

This seems to have regressed ptrace_setoptions, which calls ptrace(PTRACE_SETOPTIONS...), which is now unconditionally Err(Error::UnsupportedOperation).

I don't totally follow the reasoning for making these return UnsupportedOperation instead of letting them work if you pass the right raw pointers. Is the worry that they're usable to bypass memory safety or something?

xd009642 · 2017-06-20T17:54:54Z

@geofft do you still require input on this? If you read back through the PR there's a preference to use the higher level functions when they are available instead of the lower level ptrace call (avoids the user keeping unsafe blocks to move their data to a c_void). I'm sure @Susurrus can weigh in with any input if you still need it.

Susurrus · 2017-06-20T18:29:11Z

@xd009642 I think @geofft's point is that you cannot use ptrace_setoptions because it internally calls ptrace with a PTRACE_SETOPTIONS, which is an illegal operation. Unfortunately I didn't catch this in this PR, but this is definitely an error introduced by this PR. @xd009642 would you be able to file another PR fixing this?

geofft · 2017-06-20T18:51:01Z

Wait, doesn't the same problem also apply to ptrace_getevent and ptrace_getsiginfo? Those both call ptrace_get_data, which calls ptrace, not ffi::ptrace, which is going to do the same thing with returning UnsupportedOperation. Some test coverage would probably help here.... @xd009642 do you have code that uses these functions and do they work?

Anyway, my questions are

Why does it make sense to return UnsupportedOperation at all? Why not just let the call go through?
If the goal is to implement these at some point, does it really make sense to change the error type? (I'm just surprised to see a change to the error type, including dropping public API, as a passing change in the process of doing something else.)
Should we maybe just not expose ptrace() itself and have every single operation be a ptrace_something() type-safe function?

xd009642 · 2017-06-20T19:19:02Z

Oh I see. Dammit. I had code that used the previous pre-UnsupportedOperation version but after I got my issue debugged I didn't need it anymore. I'd personally scrap the UnsupportedOperation although I see the pros of having each function go through the ffi. I just feel that exposing the ptrace function as it was may result in some people using lower level access, but they're still using one less unsafe block in their code and every little helps :)

In terms of me raising a PR, I'm going on holiday after tomorrow so I won't be able to give it the time I'd want to for a couple of weeks. Would it be possible for @geofft to sneak the fix in with his PR? I could raise the PR I'm just conscious that there might be a longer delay in resolving it compared to if someone else did it.

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.

Susurrus reviewed Jun 1, 2017

View reviewed changes

Susurrus suggested changes Jun 1, 2017

View reviewed changes

Susurrus reviewed Jun 2, 2017

View reviewed changes

Susurrus mentioned this pull request Jun 2, 2017

[WIP] Move Ptrace{Request, Event, Options} into enums and bitflags. #461

Closed

2 tasks

Susurrus suggested changes Jun 7, 2017

View reviewed changes

Susurrus reviewed Jun 11, 2017

View reviewed changes

Susurrus suggested changes Jun 12, 2017

View reviewed changes

Susurrus approved these changes Jun 12, 2017

View reviewed changes

bors bot merged commit 43a2943 into nix-rust:master Jun 13, 2017

geofft added a commit to geofft/nix-rust that referenced this pull request Jun 20, 2017

Unbreak ptrace_setoptions (refs nix-rust#614)

dd51c4b

geofft added a commit to geofft/nix-rust that referenced this pull request Jun 20, 2017

Unbreak ptrace_setoptions (refs nix-rust#614)

80d344d

geofft added a commit to geofft/nix-rust that referenced this pull request Jun 20, 2017

Unbreak ptrace_setoptions (refs nix-rust#614)

d0fa32f

geofft mentioned this pull request Jun 20, 2017

Add WaitStatus::PtraceSyscall for use with PTRACE_O_TRACESYSGOOD #566

Merged

Susurrus mentioned this pull request Jun 24, 2017

Newtypes for uid_t, gid_t and pid_t. #629

Merged

Susurrus mentioned this pull request Jul 17, 2017

Fix special ptraces #686

Merged

Susurrus mentioned this pull request Jul 26, 2017

No conversion between Errno and std::io::Error/i32 #613

Open

paulpr0 mentioned this pull request Aug 5, 2017

Upgrade nix to 0.9 (map nix::Error to io::Error) oconnor663/os_pipe.rs#10

Merged

ericonr mentioned this pull request Jul 20, 2020

[WIP] Update to nix 0.17. ftilde/unsegen#2

Merged

Added ptrace utilities. #614

Added ptrace utilities. #614

Conversation

xd009642 commented Jun 1, 2017

Choose a reason for hiding this comment

Susurrus commented Jun 1, 2017

xd009642 commented Jun 1, 2017

xd009642 commented Jun 1, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xd009642 commented Jun 2, 2017 • edited Loading

Susurrus commented Jun 2, 2017

xd009642 commented Jun 3, 2017

Susurrus commented Jun 3, 2017

xd009642 commented Jun 3, 2017

Susurrus commented Jun 4, 2017

xd009642 commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xd009642 commented Jun 10, 2017

Choose a reason for hiding this comment

Susurrus commented Jun 11, 2017

xd009642 commented Jun 11, 2017

Susurrus commented Jun 11, 2017

xd009642 commented Jun 12, 2017

Susurrus commented Jun 12, 2017

xd009642 commented Jun 12, 2017

Susurrus commented Jun 12, 2017

xd009642 commented Jun 12, 2017

Susurrus commented Jun 12, 2017

xd009642 commented Jun 12, 2017

Susurrus commented Jun 12, 2017

Susurrus commented Jun 12, 2017

Choose a reason for hiding this comment

Susurrus commented Jun 12, 2017

xd009642 commented Jun 12, 2017

Susurrus commented Jun 12, 2017

xd009642 commented Jun 13, 2017

xd009642 commented Jun 13, 2017

Susurrus commented Jun 13, 2017

Susurrus commented Jun 13, 2017

bors bot commented Jun 13, 2017

Build succeeded

geofft commented Jun 20, 2017

xd009642 commented Jun 20, 2017

Susurrus commented Jun 20, 2017 • edited Loading

geofft commented Jun 20, 2017

xd009642 commented Jun 20, 2017

xd009642 commented Jun 2, 2017 •

edited

Loading

Susurrus commented Jun 20, 2017 •

edited

Loading