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

Wrong ptrace::write signature #1011

Open
wpovell opened this issue Jan 11, 2019 · 4 comments
Open

Wrong ptrace::write signature #1011

wpovell opened this issue Jan 11, 2019 · 4 comments

Comments

@wpovell
Copy link
Contributor

wpovell commented Jan 11, 2019

The signature of ptrace::write is:

pub fn write(pid: Pid, addr: AddressType, data: *mut c_void) -> Result<()>;

Correct me if I'm wrong, but shouldn't data be a *const c_void since we know that writing won't mutate data?

Happy to make a PR for it, but wanted to confirm my understanding.

@asomers
Copy link
Member

asomers commented Jan 12, 2019

Nix's current ptrace API reflects the underlying system API. But in this case, we probably could've made it better. At a glance, I don't see why the Linux ptrace::write couldn't have the same signature as the bsd ptrace::write, which looks like pub fn write(pid: Pid, addr: AddressType, data: c_int) -> Result<()>. CC @xd009642 , who added this code in PR #949 .

@xd009642
Copy link
Contributor

It was written to match the ptrace interface available in linux and bsd since linux lets you write the siginfo_t struct via that field while bsd only lets you write c_int. As for mutability I think you'd have to pass it in as a mutable one anyway to match the signatures as with linux that argument is also used to store returned data via the ptrace read commands. If I recall correctly the BSDs have their ptrace reads return directly instead of using the data argument.

@Ryan1729
Copy link

Ryan1729 commented Jul 7, 2020

linux lets you write the siginfo_t struct via that field

Maybe I'm misunderstanding something, but after searching for siginfo in the linux man page it seems like you can only write it when PTRACE_SETSIGINFO is passed, so I don't see what that has to do with the signature of ptrace::write, which as I understand it should be passing PTRACE_POKEDATA or possibly PTRACE_POKETEXT. since that man page says they are equivalent on linux.

Additionally, it seems to me that in an ideal world, ptrace::write should take as the data parameter the same type that is obtained after unwrapping a successful ptrace::read. As of this writing, ptrace::read returns a Result<c_long> not a Result<c_int>. Since the underlying interface for ptrace::write takes a pointer (but interprets it as a value to write to memory,) then shouldn't the common integer type be a size_t instead?

@ajwock
Copy link

ajwock commented Feb 9, 2023

Hello, I recently ran into this issue on linux. So, on linux, PTRACE_POKEDATA takes a void * as the input. However, this void * is treated as a long for this particular variant when on linux. What hurt my understanding even more was that the function is now marked unsafe which further implied that I was supposed to be passing an actual pointer. So I, not having used ptrace from C before, was unaware of this and I passed my long variable a with &mut a as *mut usize as *mut c_void when it really should have been a as *mut usize as *mut c_void. This led to several hours of me being confused as to why the value I was writing didn't seem to correlate to the address in the child process whatsoever.

While I think having a unified API as an option would be okay, I think having the option of platform-specific variants would be nice. If there were a linux version taking data: c_long and a freebsd version taking data: c_int, that would protect people from pointer coercion related bugs. Additionally, for platforms which don't require passing a true pointer, the variant should not be unsafe.

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

No branches or pull requests

5 participants