svn commit: r304928 - in head/lib/libc: amd64/sys i386/sys sys

[Andrey Chernov]

On 29.08.2016 10:07, Konstantin Belousov wrote:
> On Mon, Aug 29, 2016 at 09:58:13AM +0300, Konstantin Belousov wrote:
>> On Sun, Aug 28, 2016 at 04:09:51PM -0700, John Baldwin wrote:
>>> OTOH, given that we explicitly documented it as not being true, I suspect
>>> any applications that are using ptrace() are going off the documentation, not
>>> the implementation artifact.  Note that Linux's ptrace() documents the same
>>> requirement as before this change (caller is required to clear errno), so I
>>> doubt there is any actual software out there that expects the
>>> FreeBSD-specific behavior.  Given that and the extra maintenance overhead of
>>> having to dink with errno in assembly on X architectures, I'd rather we keep
>>> the old language in the manpage and remove the 'errno' frobbing in the system
>>> call wrappers.  To be honest, my first response to this commit was one of
>>> surprise that we modify errno directly as that is inconsistent with other
>>> system calls.  (I haven't looked to see if any other system call wrappers
>>> modify errno for non-error cases.)
>>
>> The problematic calls are PT_PEEK_I and PT_PEEK_D, as far as I understand.
>>
>> I dug into the ptrace(2) consumers, I found a lot of things using
>> it which I would not expect to use, besides usual suspects of gdb
>> lldb libunwind reptyr etc.  Most surprising was that even high-profile
>> consumers including gdb sometimes fail to check errno after PT_PEEK. On
>> the other hand, I did not found a case in gdb where errno is checked
>> after PT_PEEK but not zeroed before the syscall.
>>
>> I almost agreed with you after the reading, but then I decided to look
>> into glibc just in case.  What I found there is really fascinating.
>> From glibc/sysdeps/unix/sysv/linux:
>>   res = INLINE_SYSCALL (ptrace, 4, request, pid, addr, data);
>>   if (res >= 0 && request > 0 && request < 4)
>>     {
>>       __set_errno (0);
>>       return ret;
>>     }
>> #define PTRACE_PEEKTEXT		   1
>> #define PTRACE_PEEKDATA		   2
>> #define PTRACE_PEEKUSR		   3
>>
>> In the end, I might consider changing the ptrace wrappers into
>> consolidated C source, it would look like that
>>
>> int
>> ptrace(int request, pid_t pid, caddr_t addr, int data)
>> {
>>
>> 	errno = 0;
>> 	return (__sys_ptrace(request, pid, addr, data));
>> }
> 
> And Solaris libc, where ptrace() is the wrapper around procfs, starts its
> implementation this way:
> usr/src/lib/libc/i386/sys/ptrace.c
> 	/*
> 	 * Process the request.
> 	 */
> 	errno = 0;
> 	switch (request) {
> 	case 1:		/* PTRACE_PEEKTEXT */
> 	case 2:		/* PTRACE_PEEKDATA */
> 

No surprise since they all share the same initial implementation. You
can add NetBSD and OpenBSD too. Errno clearing as undocumented side
effect is not what we need, and every system documents that user must
clear errno.