nix icon indicating copy to clipboard operation
nix copied to clipboard

Wrong `ptrace::write` signature

Open wpovell opened this issue 6 years ago • 4 comments

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.

wpovell avatar Jan 11 '19 19:01 wpovell

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 .

asomers avatar Jan 12 '19 16:01 asomers

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.

xd009642 avatar Jan 12 '19 17:01 xd009642

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?

Ryan1729 avatar Jul 07 '20 00:07 Ryan1729

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.

ajwock avatar Feb 09 '23 14:02 ajwock

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.

Sorry for hearing that, this issue has been fixed in #2324

SteveLauC avatar May 22 '24 23:05 SteveLauC

Looks like this issue has been fixed, so close it.

SteveLauC avatar May 22 '24 23:05 SteveLauC