Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of some routines #709

marmistrz · 2017-07-26T07:54:07Z

These include:

PTRACE_TRACEME
PTRACE_CONT
PTRACE_ATTACH
PTRACE_SYSCALL

This is a part of #666, which is being split up into a couple smaller PRs.

…routines. These include: * PTRACE_TRACEME * PTRACE_CONT * PTRACE_ATTACH * PTRACE_SYSCALL

Susurrus · 2017-07-26T16:51:14Z

src/sys/ptrace.rs

+}
+
+/// Makes the `PTRACE_CONT` request to ptrace
+pub fn cont(pid: Pid) -> Result<()> {


Call this "continue" instead.

And provide better docs here.

I wanted to keep consistent with the C API: PTRACE_TRACEME => ptrace::traceme, so PTRACE_CONT => ptrace::cont

I understand why it's this way, but nix does renaming as is appropriate to make things more clear, and I think this is one of those times. So let's change it to "continue"

Sorry, continue will not work:

error: expected identifier, found keyword `continue` --> src/sys/ptrace.rs:193:8 | 193 | pub fn continue(pid: Pid) -> Result<()> { | ^^^^^^^^

Leaving this as it is.

Susurrus · 2017-07-26T16:53:24Z

src/sys/ptrace.rs

@@ -70,7 +70,7 @@ mod ffi {

 /// Performs a ptrace request. If the request in question is provided by a specialised function
 /// this function will return an unsupported operation error.
-pub fn ptrace(request: ptrace::PtraceRequest, pid: Pid, addr: *mut c_void, data: *mut c_void) -> Result<c_long> {
+pub unsafe fn ptrace(request: ptrace::PtraceRequest, pid: Pid, addr: *mut c_void, data: *mut c_void) -> Result<c_long> {


This needs to be expanded to report UnsupportedOperation for all 4 added specialized functions.

Also, what are the specific usecases of ptrace that will generate unsafety as Rust defines it (crashing isn't necessarily unsafe)? Since you pulled this out of another PR, I don't know where that discussion went.

And thus break the specialized functions: ptrace::syscall calls ptrace::ptrace with request=PTRACE_SYSCALL.

ptrace can dereference arbitrary pointer in the child process memory.

Susurrus · 2017-07-26T16:53:53Z

src/sys/ptrace.rs

+    }
+}
+
+/// Makes the `PTRACE_SYSCALL` request to ptrace


This comment should be more descriptive on the first sentence and include a really good extended description explaining what it does.

Should I copy-paste the ptrace manual? I thought that the user should be familiar with what ptrace does.

Not the whole thing, but providing a little more detail would be nice. nix documentation can also be used as a form of discovery for users who aren't familiar with the POSIX APIs. While I don't expect nix to replace the man pages or other sources of documentation, it should allow people unfamiliar with the concepts to understand a little of the functionality.

Susurrus · 2017-07-26T16:54:33Z

src/sys/ptrace.rs

+    }
+}
+
+/// Makes the `PTRACE_ATTACH` request to ptrace


Better docs, like above.

Susurrus · 2017-07-26T16:54:46Z

src/sys/ptrace.rs

@@ -140,3 +140,51 @@ pub fn setsiginfo(pid: Pid, sig: &siginfo_t) -> Result<()> {
        Err(e) => Err(e),
    }
 }
+
+/// Sets the process as traceable with `PTRACE_TRACEME`


Needs an extended description that elaborates a bit here.

Susurrus · 2017-07-26T16:55:49Z

Please don't break up PRs unless we ask you too as it makes it hard for us to follow the discussion (see my comments about safety). It also generates a lot of email traffic for a lot of us.

marmistrz · 2017-07-26T17:14:31Z

Well, the problem is that #666 has gotten far too big and complex and if I keep it in one chunk, we won't merge it for another two weeks. And I have trouble isolating the cause of tests failing, especially that they succeed on my machine.

It'll be much easier to ditch #666 (and close it when all the related changes are in) and successfully merge the changes by chunks.

Susurrus · 2017-07-26T18:42:59Z

@marmistrz Let's refocus #666 on just adding the register ptrace wrapper since that's what most of it is about. Closing it we lose the history of the discussion, which has mostly been around that single feature, so let's keep it around.

marmistrz · 2017-07-27T08:59:45Z

I'm wondering about the ptrace::cont. From manpage:

Restart the stopped tracee process.  If data is nonzero, it is interpreted as the number of a signal to be delivered to the tracee; otherwise, no signal is delivered.  Thus, for example,  the
              tracer can control whether a signal sent to the tracee is delivered or not.  (addr is ignored.)

Currently ptrace::cont uses data = 0, so delivers no signal. We could solve it in two ways:

Change the signature to

pub fn cont(pid: Pid, sig: Option<Signal>) -> Result<()> {

Create two functions:

pub fn cont(pid: Pid) -> Result<()> {
pub fn cont_signal(pid: Pid, sig: Signal) -> Result<()> {

I really think about settling with solution 2 because the most common use case is not to deliver any signal. What do you think?

Susurrus · 2017-07-27T16:18:35Z

Let's use:

pub fn cont<T: Into<Option<Signal>>>(pid: Pid, sig: T) -> Result<()>;

I don't see the need for two functions just because it's too hard to add a None argument. It's a goal of nix to stay as close as is reasonable to the C APIs.

marmistrz · 2017-07-31T14:41:37Z

It looks like we've got some problems with qemu. Should I disable the test on these arm/ppc?

Susurrus · 2017-07-31T14:49:33Z

@marmistrz If you think it's a QEMU failure, just document it and disable them for it.

marmistrz · 2017-08-01T07:39:39Z

The tests have succeeded but Github still shows them as not yet finished: https://travis-ci.org/nix-rust/nix/builds/259406655?utm_source=github_status&utm_medium=notification

Susurrus · 2017-08-01T18:02:16Z

test/sys/test_ptrace.rs

@@ -58,6 +58,7 @@ fn test_ptrace_cont() {

    // FIXME: qemu-user doesn't implement ptrace on all arches and gives ENOSYS then.
    // use it to filter out the affected platforms


Both of these comments could use proper punctuation and capitalization.

marmistrz · 2017-08-02T15:25:37Z

Is it ok now?

Susurrus · 2017-08-03T03:51:40Z

@marmistrz Not yet. You haven't expanded the list of ptrace operations that return and Unsupported error. This is also missing a Changed entry in the CHANGELOG. Should be something like "Using ptrace() with PTRACE_TRACEME, ... is no longer supported. Instead use the specialized functions traceme(), ..."

marmistrz · 2017-08-03T05:19:21Z

@Susurrus, but this will break traceme! User calls traceme, which internally calls ptrace(PTRACE_TRACEME, and whoops!

And ptrace is a very convenient function for internal use by the library

Susurrus · 2017-08-03T15:28:54Z

@marmistrz You're correct, you should change these functions to use libc::ptrace or a helper function. This is how the existing specialized wrapper functions work.

marmistrz · 2017-08-03T16:06:22Z

So you're asking me to add 4 more matches now, 6 more in #666... That's poor for readability, maintanance, that's ~3 rows of listing enum variants! And there are about 30 ptrace routines in total!

Man, it's breaking other people's code without a single, smallest warning, without a single compile-time message. If a program starts tracing another using PTRACE_SEIZE, it may not realize something's going on until the program enters an unfortunate branch!! And it's so easy to cargo update.

Let's just mark it as deprecated:

#[deprecated(since="...", note="please use specialized wrappers instead or contribute one if such doesn't exist")]

Susurrus · 2017-08-03T17:25:00Z

Yes I am. This isn't that onerous and is not a large maintenance concern. Eventually ptrace() will be completely removed from our external API and only the specialized variants will be supported. Until then, we'll be making each variant return Unsupported errors as they're implemented. #deprecated doesn't quite have the right semantics, since the function as a whole isn't deprecated, just some variants of it.

As to the breaking people's code piece, yes we are. Every release will likely have breaking changes as we improve or stabilize our APIs. nix is pre-1.0 for a reason and breakages should be expected. You should also be wary of cargo update if you use any pre-1.0 crates for exactly this reason.

marmistrz · 2017-08-04T09:09:57Z

@Susurrus It's one thing to break the API so it doesn't compile and another to introduce runtime error in the most unexpected place. API changes should be clear from the developers perspective and this, in any case, is not. And I can't stress how sneaky the change is - you may realized it weeks after updating the dependency because you simply used the supported operations.

It's just as though Rust decided that try! will panic instead of returning a result, and you've never realized it because you never had any errors before. Changing the interface is one thing, changing the semantics - another.

Moreover, Rust favors compile-time checking and falls back to runtime errors only when it's absolutely necessary.

Deprecating a 1/10 of an API because we have a 1/10 of a new API is just insane. It's just as though you had to use methods .minus(), .multiply() to multiply complex numbers but .add() threw a runtime error out of nowhere because you should a + operator.

I suggest we annotate the current state of the API in the documentation. The general ptrace function should be simply removed when all the wrappers are ready, and since we're 0.x - we can simply remove it.

Susurrus · 2017-08-09T15:41:27Z

@marmistrz I understand your viewpoint on this, but I don't see it as as big of an issue as you suggest.

That being said, I think we should go ahead and add #[deprecated(since="0.10.0", note="usages of ptrace() should be replaced with the specialized helper functions instead")].

Additionally I'm fine with you not expanding the UnsupportedOperation part of ptrace().

marmistrz · 2017-08-10T08:24:20Z

It's great we've found a compromise! I think we should document the internal functions ptrace_other and ptrace_peek (possibly in a separate pull request)

As far as I can see, ptrace_other takes care of those routines which do not return anything interesting and ptrace_peek - those who do (so peeks). But why does ptrace_peek match against Err(Error::Sys(Errno::UnknownErrno))?

I think the name ptrace_other could be clearer. I had an idea to create a second function, named, for example, fn ptrace_noretval(...) -> Result<()>, which would save us the .map(|_| ()) in the specialized wrappers. And remove ptrace_other just when we decide to remove ptrace. What do you think?

Susurrus · 2017-08-10T16:50:59Z

@marmistrz I think you're on the right track for cleaning up these helper functions. I don't think you need feedback from me, so why don't you go ahead and clean those up. You can put it as the first commit in this series or file a new PR, whatever you want.

As to why UnknownErrno is checked for, I'm not certain. It was there in the first implementation added in a0fc607.

marmistrz · 2017-08-11T14:20:52Z

Sure. I'll do this in a separate PR. Let's finally have it merged :)

Susurrus · 2017-08-11T15:29:51Z

Thanks for your hard work pushing on this @marmistrz! Love that we have more tests for this too.

bors r+

709: Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of some routines r=Susurrus These include: * PTRACE_TRACEME * PTRACE_CONT * PTRACE_ATTACH * PTRACE_SYSCALL This is a part of #666, which is being split up into a couple smaller PRs.

bors · 2017-08-11T16:52:45Z

Build succeeded

Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of source …

b03011f

…routines. These include: * PTRACE_TRACEME * PTRACE_CONT * PTRACE_ATTACH * PTRACE_SYSCALL

marmistrz changed the title ~~Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of source routines~~ Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of some routines Jul 26, 2017

Susurrus suggested changes Jul 26, 2017

View reviewed changes

Improve the documentation of wrappers.

843eeb0

marmistrz added 2 commits July 31, 2017 10:50

support delivering a signal with ptrace::cont

0fbac87

Add tests for ptrace::cont

5510bf2

marmistrz added 3 commits July 31, 2017 17:04

Possibly fix tests failing on qemu architectures.

9e0d241

Add a line break.

c65b5ba

Extra doc

cf3c2ad

Susurrus reviewed Aug 1, 2017

View reviewed changes

Reword the comment.

932f6e0

marmistrz added 3 commits August 3, 2017 10:29

Update changelog

f4a0099

Refactor the changelog

9ad235c

Merge remote-tracking branch 'upstream/master' into ptrace-noreg

ebcedb3

marmistrz mentioned this pull request Aug 7, 2017

Add more ptrace_* wrappers #666

Closed

marmistrz added 3 commits August 10, 2017 10:13

Deprecate ptrace.

0587977

Merge remote-tracking branch 'upstream/master' into ptrace-noreg

d8968f4

Remove the incorrect doc comment.

abcec0f

Susurrus approved these changes Aug 10, 2017

View reviewed changes

bors bot merged commit abcec0f into nix-rust:master Aug 11, 2017

		@@ -58,6 +58,7 @@ fn test_ptrace_cont() {

		// FIXME: qemu-user doesn't implement ptrace on all arches and gives ENOSYS then.
		// use it to filter out the affected platforms

Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of some routines #709

Mark nix::sys::ptrace::ptrace as unsafe, add safe variants of some routines #709

Uh oh!

Conversation

marmistrz commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmistrz Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmistrz Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Susurrus commented Jul 26, 2017

Uh oh!

marmistrz commented Jul 26, 2017

Uh oh!

Susurrus commented Jul 26, 2017

Uh oh!

marmistrz commented Jul 27, 2017 • edited by Susurrus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susurrus commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmistrz commented Jul 31, 2017

Uh oh!

Susurrus commented Jul 31, 2017

Uh oh!

marmistrz commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmistrz commented Aug 2, 2017

Uh oh!

Susurrus commented Aug 3, 2017

Uh oh!

marmistrz commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susurrus commented Aug 3, 2017

Uh oh!

marmistrz commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susurrus commented Aug 3, 2017

Uh oh!

marmistrz commented Aug 4, 2017

Uh oh!

Susurrus commented Aug 9, 2017

Uh oh!

marmistrz commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susurrus commented Aug 10, 2017

Uh oh!

marmistrz commented Aug 11, 2017

Uh oh!

Susurrus commented Aug 11, 2017

marmistrz commented Jul 26, 2017 •

edited

Loading

marmistrz Jul 27, 2017 •

edited

Loading

marmistrz Jul 26, 2017 •

edited

Loading

marmistrz commented Jul 27, 2017 •

edited by Susurrus

Loading

Susurrus commented Jul 27, 2017 •

edited

Loading

marmistrz commented Aug 1, 2017 •

edited

Loading

marmistrz commented Aug 3, 2017 •

edited

Loading

marmistrz commented Aug 3, 2017 •

edited

Loading

marmistrz commented Aug 10, 2017 •

edited

Loading