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.

Sign up for GitHub

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

Jump to bottom

Add PTRACE_INTERRUPT call as ptrace::interrupt(pid) #1422

Merged
merged 1 commit into from Aug 9, 2021
Merged

Add PTRACE_INTERRUPT call as ptrace::interrupt(pid) #1422

merged 1 commit into from Aug 9, 2021

Conversation

blaind
Copy link
Contributor

@blaind blaind commented Apr 11, 2021

I've based the test on fn test_ptrace_cont. Removed some parts, but not 100% sure what's the proper way of testing in nix context.

From ptrace-man page:

       PTRACE_INTERRUPT (since Linux 3.4)
              Stop a tracee.  If the tracee is running or sleeping in
              kernel space and PTRACE_SYSCALL is in effect, the system
              call is interrupted and syscall-exit-stop is reported.
              (The interrupted system call is restarted when the tracee
              is restarted.)  If the tracee was already stopped by a
              signal and PTRACE_LISTEN was sent to it, the tracee stops
              with PTRACE_EVENT_STOP and WSTOPSIG(status) returns the
              stop signal.  If any other ptrace-stop is generated at the
              same time (for example, if a signal is sent to the
              tracee), this ptrace-stop happens.  If none of the above
              applies (for example, if the tracee is running in user
              space), it stops with PTRACE_EVENT_STOP with
              WSTOPSIG(status) == SIGTRAP.  PTRACE_INTERRUPT only works
              on tracees attached by PTRACE_SEIZE.

blaind
blaind commented Apr 11, 2021
View reviewed changes
test/sys/test_ptrace.rs
Parent { child } => {
ptrace::seize(child, ptrace::Options::PTRACE_O_TRACESYSGOOD).unwrap();
ptrace::interrupt(child).unwrap();
assert_eq!(waitpid(child, None), Ok(WaitStatus::PtraceEvent(child, Signal::SIGTRAP, 128)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I verified, interrupt will fire the PtraceEvent. Not sure about the third parameter 128, is it same over all systems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Where did you copy the 128 parameter from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took it from the waitpid(...) return state. Seems that internally it converts status through

#[cfg(any(target_os = "android", target_os = "linux"))]
fn stop_additional(status: i32) -> c_int {
    (status >> 16) as c_int
}

@blaind blaind mentioned this pull request Apr 11, 2021
Open
asomers
asomers reviewed Apr 17, 2021
View reviewed changes
test/sys/test_ptrace.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented May 1, 2021

@blaind could you please fix the compile failures?

@blaind
Copy link
Contributor Author

blaind commented May 2, 2021

@blaind could you please fix the compile failures?

I had a look, the builds are failing now because of missing variables on various platforms. On android, libc::{PTRACE_SEIZE, PTRACE_INTERRUPT}; are missing, and on *BSD (mips?) a lot more failures.

I guess one way to fix is to go back with feature gate (#[cfg(all(target_os = "linux", not(any(target_arch = "mips", target_arch = "mips64"))))]). Any other ideas?

@asomers
Copy link
Member

asomers commented May 16, 2021

It's failing on non-Linux platforms because stuff like PTRACE_SEIZE is Linux-specific. That's why they were feature gated to begin with. I only asked about removing the mips restriction, not the OS restriction.

@blaind
Copy link
Contributor Author

blaind commented May 16, 2021

It's failing on non-Linux platforms because stuff like PTRACE_SEIZE is Linux-specific. That's why they were feature gated to begin with. I only asked about removing the mips restriction, not the OS restriction.

I mistakenly took architecture to mean OS as well. Anyway, just pushed a new commit that might fix it. Testing is semiwhat slow, as I can not test locally.

@ranweiler
Copy link
Contributor

Note, this is a duplicate of #1279 (which omits tests). Whichever PR gets merged, the other should be closed.

asomers
asomers requested changes Jul 29, 2021
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rebase onto master to fix the conflicts?

@blaind
Copy link
Contributor Author

blaind commented Aug 8, 2021

Could you please rebase onto master to fix the conflicts?

Done

asomers
asomers requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, everything looks good now. Would you mind squashing your commits?

asomers
asomers requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

CHANGELOG.md
@@ -90,6 +90,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
- Added `sendfile64` (#[1439](https://github.com/nix-rust/nix/pull/1439))
- Added `MS_LAZYTIME` to `MsFlags`
(#[1437](https://github.com/nix-rust/nix/pull/1437))
- Added `ptrace::interrupt` method for platforms that support `PTRACE_INTERRUPT`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot. I just noticed that your CHANGELOG entry got moved into the wrong part of the file when you rebased. You'll have to move it up to the top.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I forgot to stop bors when I found the CHANGELOG. problem. Don't worry; I'll take care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry about that. Did not notice when rebasing with master. Thanks for fixing!

@bors
Copy link
Contributor

bors bot commented Aug 9, 2021

Build succeeded:

@bors bors bot merged commit c5590df into nix-rust:master Aug 9, 2021
asomers added a commit that referenced this pull request Aug 9, 2021
@asomers
[skip ci] fix CHANGELOG after PR #1422
68488a4
@asomers asomers mentioned this pull request Aug 9, 2021
Closed
@coolreader18 coolreader18 mentioned this pull request Aug 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@blaind @asomers @ranweiler