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

[John Baldwin]

On Monday, August 29, 2016 10:10:35 PM Konstantin Belousov wrote:
> On Mon, Aug 29, 2016 at 09:16:29PM +0300, Andrey Chernov wrote:
> > Previous text is documented everywhere and describing usual good
> > practice, so it should remains in place, in that means r304928 should be
> > reverted, because replace proper way of doing things with obsoleted
> > feature description.
> > 
> > What I suggest is not _replace_ old text with new, but _add_ new text as
> > describing current and not recommended way, with the direct mention that
> > this feature is obsoleted and not recommended for relay on it.
> > 
> 
> diff --git a/lib/libc/sys/ptrace.2 b/lib/libc/sys/ptrace.2
> index 48802f4..d406efc 100644
> --- a/lib/libc/sys/ptrace.2
> +++ b/lib/libc/sys/ptrace.2
> @@ -2,7 +2,7 @@
>  .\"  $NetBSD: ptrace.2,v 1.2 1995/02/27 12:35:37 cgd Exp $
>  .\"
>  .\" This file is in the public domain.
> -.Dd August 28, 2016
> +.Dd August 29, 2016
>  .Dt PTRACE 2
>  .Os
>  .Sh NAME
> @@ -900,19 +900,29 @@ argument is ignored.
>  .Pp
>  Additionally, other machine-specific requests can exist.
>  .Sh RETURN VALUES
> +Most requests return 0 on success and \-1 on error.
>  Some requests can cause
>  .Fn ptrace
>  to return
>  \-1
> -as a non-error value; to disambiguate,
> +as a non-error value, among them are
> +.Dv PT_READ_I
> +and
> +.Dv PT_READ_D ,
> +which return the value read from the process memory on success.
> +To disambiguate,
>  .Va errno
> -is set to 0 in the libc wrapper for the
> -.Fn ptrace
> -system call and
> +can be set to 0 before the call and checked afterwards.
> +.Pp
> +Current
>  .Fn ptrace
> -callers can reliably check
> +implementation always sets
> +.Va errno
> +to 0 before calling into kernel, both for historic reasons and for
> +consistency with other operating systems.
> +It is recommended to assign zero to
>  .Va errno
> -for non-zero value afterwards.
> +explicitely for future compatibility.
>  .Sh ERRORS
>  The
>  .Fn ptrace

Thanks, I think this looks good.

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 29.08.2016 22:10, Konstantin Belousov wrote:
> On Mon, Aug 29, 2016 at 09:16:29PM +0300, Andrey Chernov wrote:
>> Previous text is documented everywhere and describing usual good
>> practice, so it should remains in place, in that means r304928 should be
>> reverted, because replace proper way of doing things with obsoleted
>> feature description.
>>
>> What I suggest is not _replace_ old text with new, but _add_ new text as
>> describing current and not recommended way, with the direct mention that
>> this feature is obsoleted and not recommended for relay on it.
>>
> 
> diff --git a/lib/libc/sys/ptrace.2 b/lib/libc/sys/ptrace.2
> index 48802f4..d406efc 100644
> --- a/lib/libc/sys/ptrace.2
> +++ b/lib/libc/sys/ptrace.2
> @@ -2,7 +2,7 @@
>  .\"  $NetBSD: ptrace.2,v 1.2 1995/02/27 12:35:37 cgd Exp $
>  .\"
>  .\" This file is in the public domain.
> -.Dd August 28, 2016
> +.Dd August 29, 2016
>  .Dt PTRACE 2
>  .Os
>  .Sh NAME
> @@ -900,19 +900,29 @@ argument is ignored.
>  .Pp
>  Additionally, other machine-specific requests can exist.
>  .Sh RETURN VALUES
> +Most requests return 0 on success and \-1 on error.
>  Some requests can cause
>  .Fn ptrace
>  to return
>  \-1
> -as a non-error value; to disambiguate,
> +as a non-error value, among them are
> +.Dv PT_READ_I
> +and
> +.Dv PT_READ_D ,
> +which return the value read from the process memory on success.
> +To disambiguate,
>  .Va errno
> -is set to 0 in the libc wrapper for the
> -.Fn ptrace
> -system call and
> +can be set to 0 before the call and checked afterwards.
> +.Pp
> +Current
>  .Fn ptrace
> -callers can reliably check
> +implementation always sets
> +.Va errno
> +to 0 before calling into kernel, both for historic reasons and for
> +consistency with other operating systems.
> +It is recommended to assign zero to
>  .Va errno
> -for non-zero value afterwards.
> +explicitely for future compatibility.
>  .Sh ERRORS
>  The
>  .Fn ptrace
> 

Ok.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Benjamin Kaduk]

On Mon, Aug 29, 2016 at 2:10 PM, Konstantin Belousov 
wrote:

> On Mon, Aug 29, 2016 at 09:16:29PM +0300, Andrey Chernov wrote:
> > Previous text is documented everywhere and describing usual good
> > practice, so it should remains in place, in that means r304928 should be
> > reverted, because replace proper way of doing things with obsoleted
> > feature description.
> >
> > What I suggest is not _replace_ old text with new, but _add_ new text as
> > describing current and not recommended way, with the direct mention that
> > this feature is obsoleted and not recommended for relay on it.
> >
>
> diff --git a/lib/libc/sys/ptrace.2 b/lib/libc/sys/ptrace.2
> index 48802f4..d406efc 100644
> --- a/lib/libc/sys/ptrace.2
> +++ b/lib/libc/sys/ptrace.2
> @@ -2,7 +2,7 @@
>  .\"$NetBSD: ptrace.2,v 1.2 1995/02/27 12:35:37 cgd Exp $
>  .\"
>  .\" This file is in the public domain.
> -.Dd August 28, 2016
> +.Dd August 29, 2016
>  .Dt PTRACE 2
>  .Os
>  .Sh NAME
> @@ -900,19 +900,29 @@ argument is ignored.
>  .Pp
>  Additionally, other machine-specific requests can exist.
>  .Sh RETURN VALUES
> +Most requests return 0 on success and \-1 on error.
>  Some requests can cause
>  .Fn ptrace
>  to return
>  \-1
> -as a non-error value; to disambiguate,
> +as a non-error value, among them are
> +.Dv PT_READ_I
> +and
> +.Dv PT_READ_D ,
> +which return the value read from the process memory on success.
> +To disambiguate,
>  .Va errno
> -is set to 0 in the libc wrapper for the
> -.Fn ptrace
> -system call and
> +can be set to 0 before the call and checked afterwards.
> +.Pp
> +Current
>

The current


>  .Fn ptrace
> -callers can reliably check
> +implementation always sets
> +.Va errno
> +to 0 before calling into kernel, both for historic reasons and for
>

the kernel


> +consistency with other operating systems.
> +It is recommended to assign zero to
>  .Va errno
> -for non-zero value afterwards.
> +explicitely for future compatibility.
>

"explicitly" (only one 'e')

Maybe forward compatibility instead of future compatibility, but either is
fine.

Feel free to commit with reviewed-by: bjk

-Ben


>  .Sh ERRORS
>  The
>  .Fn ptrace
> ___
> svn-src-...@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Mon, Aug 29, 2016 at 09:16:29PM +0300, Andrey Chernov wrote:
> Previous text is documented everywhere and describing usual good
> practice, so it should remains in place, in that means r304928 should be
> reverted, because replace proper way of doing things with obsoleted
> feature description.
> 
> What I suggest is not _replace_ old text with new, but _add_ new text as
> describing current and not recommended way, with the direct mention that
> this feature is obsoleted and not recommended for relay on it.
> 

diff --git a/lib/libc/sys/ptrace.2 b/lib/libc/sys/ptrace.2
index 48802f4..d406efc 100644
--- a/lib/libc/sys/ptrace.2
+++ b/lib/libc/sys/ptrace.2
@@ -2,7 +2,7 @@
 .\"$NetBSD: ptrace.2,v 1.2 1995/02/27 12:35:37 cgd Exp $
 .\"
 .\" This file is in the public domain.
-.Dd August 28, 2016
+.Dd August 29, 2016
 .Dt PTRACE 2
 .Os
 .Sh NAME
@@ -900,19 +900,29 @@ argument is ignored.
 .Pp
 Additionally, other machine-specific requests can exist.
 .Sh RETURN VALUES
+Most requests return 0 on success and \-1 on error.
 Some requests can cause
 .Fn ptrace
 to return
 \-1
-as a non-error value; to disambiguate,
+as a non-error value, among them are
+.Dv PT_READ_I
+and
+.Dv PT_READ_D ,
+which return the value read from the process memory on success.
+To disambiguate,
 .Va errno
-is set to 0 in the libc wrapper for the
-.Fn ptrace
-system call and
+can be set to 0 before the call and checked afterwards.
+.Pp
+Current
 .Fn ptrace
-callers can reliably check
+implementation always sets
+.Va errno
+to 0 before calling into kernel, both for historic reasons and for
+consistency with other operating systems.
+It is recommended to assign zero to
 .Va errno
-for non-zero value afterwards.
+explicitely for future compatibility.
 .Sh ERRORS
 The
 .Fn ptrace
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 29.08.2016 21:16, Andrey Chernov wrote:
> On 29.08.2016 21:04, Konstantin Belousov wrote:
>> On Mon, Aug 29, 2016 at 08:46:47PM +0300, Andrey Chernov wrote:
>>> Either we implement this wrapper or left all things as is, we need to
>>> document internal errno clearing additionally, to not make people wonder
>>> why errno becomes 0, probably with the mention that program should not
>>> relay on this obsoleted implementation feature.
>> It was done in r304928.  John want to revert this change, it seems.
>>
> Previous text is documented everywhere and describing usual good
> practice, so it should remains in place, in that means r304928 should be
> reverted, because replace proper way of doing things with obsoleted
> feature description.
> 
> What I suggest is not _replace_ old text with new, but _add_ new text as
> describing current and not recommended way, with the direct mention that
> this feature is obsoleted and not recommended for relay on it.

I.e. add to old text something like this (my English is bad, so it
probably needs correction):
"Currently ptrace(2) always set errno to 0 at the start, but portable
program should not relay on this obsoleted feature".

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 29.08.2016 21:04, Konstantin Belousov wrote:
> On Mon, Aug 29, 2016 at 08:46:47PM +0300, Andrey Chernov wrote:
>> Either we implement this wrapper or left all things as is, we need to
>> document internal errno clearing additionally, to not make people wonder
>> why errno becomes 0, probably with the mention that program should not
>> relay on this obsoleted implementation feature.
> It was done in r304928.  John want to revert this change, it seems.
> 
Previous text is documented everywhere and describing usual good
practice, so it should remains in place, in that means r304928 should be
reverted, because replace proper way of doing things with obsoleted
feature description.

What I suggest is not _replace_ old text with new, but _add_ new text as
describing current and not recommended way, with the direct mention that
this feature is obsoleted and not recommended for relay on it.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Mon, Aug 29, 2016 at 09:51:50AM -0700, John Baldwin wrote:
> On Monday, August 29, 2016 09:58:13 AM Konstantin Belousov wrote:
> > 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.
> 
> So the consumers are generally doing the right thing.
I only looked at gdb in details, other code is painful to read.

> Certainly I think having a C wrapper like this makes more sense than
> doing it all in assembly N times.  I would probably prefer to keep the
> manpage language the way it is though.
So do you want to revert man page bits ?

> 
> If we are really worried about compat, we could bump the ptrace symver
> and use the function above as the FBSD_1.0 version perhaps.
This does not solve anything.  Old-time compiled consumers would get
the expected interface, while the same consumers compiled anew get
incompatible interface.

Recent dirname/basename change demostrates the same problem.

On Mon, Aug 29, 2016 at 08:46:47PM +0300, Andrey Chernov wrote:
> Either we implement this wrapper or left all things as is, we need to
> document internal errno clearing additionally, to not make people wonder
> why errno becomes 0, probably with the mention that program should not
> relay on this obsoleted implementation feature.
It was done in r304928.  John want to revert this change, it seems.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 29.08.2016 19:51, John Baldwin wrote:
>> int
>> ptrace(int request, pid_t pid, caddr_t addr, int data)
>> {
>>
>>  errno = 0;
>>  return (__sys_ptrace(request, pid, addr, data));
>> }
> 
> Certainly I think having a C wrapper like this makes more sense than
> doing it all in assembly N times.  I would probably prefer to keep the
> manpage language the way it is though.

Either we implement this wrapper or left all things as is, we need to
document internal errno clearing additionally, to not make people wonder
why errno becomes 0, probably with the mention that program should not
relay on this obsoleted implementation feature.

IMHO, it will be better to not clear errno at all, considering 'before
call' way documented everywhere and consumers behavior analyzed by kib@

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[John Baldwin]

On Monday, August 29, 2016 09:58:13 AM 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.

So the consumers are generally doing the right thing.

> 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));
> }

Certainly I think having a C wrapper like this makes more sense than
doing it all in assembly N times.  I would probably prefer to keep the
manpage language the way it is though.

If we are really worried about compat, we could bump the ptrace symver
and use the function above as the FBSD_1.0 version perhaps.

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 29.08.2016 18:34, Konstantin Belousov wrote:
> On Mon, Aug 29, 2016 at 06:05:50PM +0300, Andrey Chernov wrote:
>> 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.
> Of course they do not share initial implementation. Solaris ptrace(2) is
> the recent clean room implementation of the interface on top of their
> procfs(4).  Glibc provides the code specific to linux, and they only
> clear errno for requests which might return -1 legitimately, and only do
> it after.
> 
> I would understand your argument if I referenced Open/NetBSD, but I did not.
> 

Of course I don't mean that they share the same code, it is impossible.
By sharing I mean they look at the same initial implementation (or its
already existent forks), writing their own code, so this errno clearing
piece becomes common as becomes common that really documented another
way is.

But all this is history. Now we have
a) Undocumented errno clearing (as bare minimum).
b) Inconsistency with other libc functions and syscalls related to
forced errno clearing (in wider scope).

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Mon, Aug 29, 2016 at 06:05:50PM +0300, Andrey Chernov wrote:
> 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.
Of course they do not share initial implementation. Solaris ptrace(2) is
the recent clean room implementation of the interface on top of their
procfs(4).  Glibc provides the code specific to linux, and they only
clear errno for requests which might return -1 legitimately, and only do
it after.

I would understand your argument if I referenced Open/NetBSD, but I did not.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: 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.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

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 */

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

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_PEEKTEXT1
#define PTRACE_PEEKDATA2
#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));
}
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[John Baldwin]

On Sunday, August 28, 2016 04:52:10 AM Konstantin Belousov wrote:
> On Sun, Aug 28, 2016 at 04:25:46AM +0300, Andrey Chernov wrote:
> > On 28.08.2016 4:15, Konstantin Belousov wrote:
> > >> POSIX: "No function in this volume of POSIX.1-2008 shall set errno to 
> > >> zero."
> > > I am quite curious where ptrace(2) is defined by POSIX.
> > 
> > POSIX just repeats C99 statement for its own functions, supporting this
> > rule too, but C99 rule is more general and related to any library functions.
> > 
> > >> POSIX: "For each thread of a process, the value of errno shall not be
> > >> affected by function calls or assignments to errno by other threads."
> > > And ?  What should the citation add new to the substance
> > > of the code change ?
> > 
> > This is for your comment "On both i386 and amd64, the errno symbol was
> > directly  referenced, which only works correctly in single-threaded
> > process."
> I still do not understand what you want to say there. Errno as the
> symbol existing in the symbol table of libc, gives 'POSIX errno' value
> for the main thread. Preprocessor definition converts C language
> accesses to errno into some indirections which result in accesses to
> per-thread errno location. The bug in x86 asm code was due to direct
> usage of errno.
> 
> What POSIX requires from the C-level errno symbol does not define a
> semantic for the memory location pointed to by the errno sym-table
> symbol.
> 
> And amusingly, all other arches did it right, except aarch64 and risc-v
> copied from aarch64.  They lack the wrapper at all, I wrote aarch64
> ptrace.S already.

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.)

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 8:28, Bruce Evans wrote:
>> How hard it will be to bring ptrace() to what C99 expects? Perhaps now
>> time is suited well to change some obsoleted things.
> 
> This should be safe to change, since portable applications like gdb can't
> assume that the implemementation clobbers errno for them.

It looks safe for me too.

> Even FreeBSD's man page doesn't document the FreeBSD behaviour.  It
> documents, with poor wording, that applications must set errno as usual:
> 
> %%%
> RETURN VALUES
>  Some requests can cause ptrace() to return -1 as a non-error value; to
>  disambiguate, errno can be set to 0 before the call and checked
>  afterwards.
> %%%
> 
> The poor wording is just "errno can be set to 0".  It _must_ be set to 0.
> Also, the function gurantees to not clobber errno so that this checking
> is guaranteed to work.

Yes, I already mention this thing in my hour ago (related to your
answer) reply. We even don't have documented that ptrace() itself
overwrites errno, but document usual practice by setting it to 0 by its
user instead.

>> "conforming implementation may have extensions (including additional
>> library functions), provided they do not alter the behavior of any
>> strictly conforming program.3)"
>>
>> ptrace() is extension (additional library function) so can't set errno
>> to 0 (it breaks strictly conforming program).
> 
> Use of ptrace() makes a program very far from stricty conforming.  Only
> quality of implementation requires ptrace() to follow the usual rules.

It was terms mistake from my side, I already correct myself in my hour
ago reply.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Bruce Evans]

On Sun, 28 Aug 2016, Andrey Chernov wrote:


On 28.08.2016 4:52, Konstantin Belousov wrote:

POSIX: "For each thread of a process, the value of errno shall not be
affected by function calls or assignments to errno by other threads."

And ?  What should the citation add new to the substance
of the code change ?


This is for your comment "On both i386 and amd64, the errno symbol was
directly  referenced, which only works correctly in single-threaded
process."

I still do not understand what you want to say there. Errno as the
symbol existing in the symbol table of libc, gives 'POSIX errno' value
for the main thread. Preprocessor definition converts C language
accesses to errno into some indirections which result in accesses to
per-thread errno location. The bug in x86 asm code was due to direct
usage of errno.


This particular quote is not describing a problem, it supports your change.


I know that POSIX requires that POSIX-defined functions did not modified
errno except on error,


POSIX don't say it. You may modify errno to any value besides 0 while
returning success from the function excepting only those functions where
POSIX directly states they can't modify errno. I.e. only 0 is disallowed
in all cases.


POSIX seems to be very deficient in stating which functions can't modify
errno.  It doesn't say this clearly even for strtol().  C90 and later
C standards say this clearly for strtol() but not many other functions
since not many other functions need this in plain C.  POSIX only says this
indirectly even for strtol() by saying that it defers to the C standard
unless stated otherwise.  But POSIX has many functions that need this
statement, starting with read() with sizes > SSIZE_MAX on implementations
that support such sizes.  (read() is very hard to use if you use such
sizes with it.  v7 had the correct arg type (int) to disallow such sizes.
Now read() is only safe to use by avoiding such sizes.  This is easy to
do in applications but not in libraries.)


I agree that it would be more
consistent for ptrace(2) to not do that as well, but the behaviour is
already there for 35 years and I do not view the 'consistency' as a serious
reason to break ABI and introduce random failures for innocent consumers
of it.


How hard it will be to bring ptrace() to what C99 expects? Perhaps now
time is suited well to change some obsoleted things.


This should be safe to change, since portable applications like gdb can't
assume that the implemementation clobbers errno for them.

Even FreeBSD's man page doesn't document the FreeBSD behaviour.  It
documents, with poor wording, that applications must set errno as usual:

%%%
RETURN VALUES
 Some requests can cause ptrace() to return -1 as a non-error value; to
 disambiguate, errno can be set to 0 before the call and checked
 afterwards.
%%%

The poor wording is just "errno can be set to 0".  It _must_ be set to 0.
Also, the function gurantees to not clobber errno so that this checking
is guaranteed to work.

FreeBSD's man page for strtol says nothing at all about either setting
errno before calls or the C90+ guarantees that make this useful.


"conforming implementation may have extensions (including additional
library functions), provided they do not alter the behavior of any
strictly conforming program.3)"

ptrace() is extension (additional library function) so can't set errno
to 0 (it breaks strictly conforming program).


Use of ptrace() makes a program very far from stricty conforming.  Only
quality of implementation requires ptrace() to follow the usual rules.

Bruce
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 5:33, Andrey Chernov wrote:
> "conforming implementation may have extensions (including additional
> library functions), provided they do not alter the behavior of any
> strictly conforming program.3)"
> 
> ptrace() is extension (additional library function) so can't set errno
> to 0 (it breaks strictly conforming program).

Sorry for misguiding in this particular part. I confuse the "strictly
conforming program" with the "conforming program" term used there too.
"Strictly conforming program" can't call ptrace().

In any case our own ptrace(2) manpage suggest to set errno to 0 manually
_before_ ptrace() call and do not relay on ptrace() to do it by itself:

"Some requests can cause ptrace() to return -1 as a non-error value; to
disambiguate, errno can be set to 0 before the call and checked afterwards."

It will be better to stay common policy about errno even for extensions
to not keep exclusions in the mind.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 4:52, Konstantin Belousov wrote:
 POSIX: "For each thread of a process, the value of errno shall not be
 affected by function calls or assignments to errno by other threads."
>>> And ?  What should the citation add new to the substance
>>> of the code change ?
>>
>> This is for your comment "On both i386 and amd64, the errno symbol was
>> directly  referenced, which only works correctly in single-threaded
>> process."
> I still do not understand what you want to say there. Errno as the
> symbol existing in the symbol table of libc, gives 'POSIX errno' value
> for the main thread. Preprocessor definition converts C language
> accesses to errno into some indirections which result in accesses to
> per-thread errno location. The bug in x86 asm code was due to direct
> usage of errno.

This particular quote is not describing a problem, it supports your change.

> I know that POSIX requires that POSIX-defined functions did not modified
> errno except on error, 

POSIX don't say it. You may modify errno to any value besides 0 while
returning success from the function excepting only those functions where
POSIX directly states they can't modify errno. I.e. only 0 is disallowed
in all cases.

> I agree that it would be more
> consistent for ptrace(2) to not do that as well, but the behaviour is
> already there for 35 years and I do not view the 'consistency' as a serious
> reason to break ABI and introduce random failures for innocent consumers
> of it.

How hard it will be to bring ptrace() to what C99 expects? Perhaps now
time is suited well to change some obsoleted things.

>> Already done: ptrace == "any library function".
> *Shaking head*
> 
> 'Library functions' references in the context of C99/C11 are implicitely
> limited to the functions defined by the chapter 7 Library of the standard.

No, they are limited to the libraries described in ISO/IEC International
Standards family which have C standard library among them. C standard
library described in ANSI C standard which is:
ISO/IEC (1999). ISO/IEC 9899:1999(E): Programming Languages.
libc is C standard library with extensions, and C99 directly says about
them:

"conforming implementation may have extensions (including additional
library functions), provided they do not alter the behavior of any
strictly conforming program.3)"

ptrace() is extension (additional library function) so can't set errno
to 0 (it breaks strictly conforming program).

> To play this sillyness to the end, please answer whether I am allowed to
> provide static functions definitions in the libraries headers ? E.g. clause
> 7.1.2 6 of C11 says:
> Any declaration of a library function shall have external linkage.

You can, as long as your static function (i.e. extension) do not alter
the behavior of any strictly conforming program (see the quote above).


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Sun, Aug 28, 2016 at 04:25:46AM +0300, Andrey Chernov wrote:
> On 28.08.2016 4:15, Konstantin Belousov wrote:
> >> POSIX: "No function in this volume of POSIX.1-2008 shall set errno to 
> >> zero."
> > I am quite curious where ptrace(2) is defined by POSIX.
> 
> POSIX just repeats C99 statement for its own functions, supporting this
> rule too, but C99 rule is more general and related to any library functions.
> 
> >> POSIX: "For each thread of a process, the value of errno shall not be
> >> affected by function calls or assignments to errno by other threads."
> > And ?  What should the citation add new to the substance
> > of the code change ?
> 
> This is for your comment "On both i386 and amd64, the errno symbol was
> directly  referenced, which only works correctly in single-threaded
> process."
I still do not understand what you want to say there. Errno as the
symbol existing in the symbol table of libc, gives 'POSIX errno' value
for the main thread. Preprocessor definition converts C language
accesses to errno into some indirections which result in accesses to
per-thread errno location. The bug in x86 asm code was due to direct
usage of errno.

What POSIX requires from the C-level errno symbol does not define a
semantic for the memory location pointed to by the errno sym-table
symbol.

And amusingly, all other arches did it right, except aarch64 and risc-v
copied from aarch64.  They lack the wrapper at all, I wrote aarch64
ptrace.S already.

> 
> >> C99 statement sounds stricter:
> >> "The value of errno is zero at program startup, but is never set to zero
> >> by any library function. 176)"
> >> And syscall is not different from library function from C99 point of view.
> > Point me to a single line in C99 which mentions ptrace().
> > 
> > Do you understand what did the commit changed, and what it did not ?
> > Setting errno to zero before the syscall was the existing behaviour
> > before the change, and I did not modified anything there. But previous
> > wrapper set errno to zero in main thread even if called from some other
> > thread, which was the bug fixed.
> 
> If you may notice, I don't blame you and don't say that you introduce
> setting errno to 0. I just want to bring your attention to the problem
> while you are in that area and familiar with it.
> 
I know that POSIX requires that POSIX-defined functions did not modified
errno except on error, but it cannot require anything from functions
which are not defined by the standard.  I agree that it would be more
consistent for ptrace(2) to not do that as well, but the behaviour is
already there for 35 years and I do not view the 'consistency' as a serious
reason to break ABI and introduce random failures for innocent consumers
of it.

On Sun, Aug 28, 2016 at 04:37:11AM +0300, Andrey Chernov wrote:
> On 28.08.2016 4:25, Andrey Chernov wrote:
> >> Point me to a single line in C99 which mentions ptrace().
> 
> Already done: ptrace == "any library function".
*Shaking head*

'Library functions' references in the context of C99/C11 are implicitely
limited to the functions defined by the chapter 7 Library of the standard.

To play this sillyness to the end, please answer whether I am allowed to
provide static functions definitions in the libraries headers ? E.g. clause
7.1.2 6 of C11 says:
Any declaration of a library function shall have external linkage.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 4:37, Andrey Chernov wrote:
> On 28.08.2016 4:25, Andrey Chernov wrote:
>>> Point me to a single line in C99 which mentions ptrace().
> 
> Already done: ptrace == "any library function".

To elaborate it more, C99 does not have finite list of library
functions, but it says about standard libraries in general (not user
libraries), and ptrace() belongs to standard library according to its
manpage:
Standard C Library (libc, -lc)

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 4:25, Andrey Chernov wrote:
>> Point me to a single line in C99 which mentions ptrace().

Already done: ptrace == "any library function".


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 4:15, Konstantin Belousov wrote:
>> POSIX: "No function in this volume of POSIX.1-2008 shall set errno to zero."
> I am quite curious where ptrace(2) is defined by POSIX.

POSIX just repeats C99 statement for its own functions, supporting this
rule too, but C99 rule is more general and related to any library functions.

>> POSIX: "For each thread of a process, the value of errno shall not be
>> affected by function calls or assignments to errno by other threads."
> And ?  What should the citation add new to the substance
> of the code change ?

This is for your comment "On both i386 and amd64, the errno symbol was
directly  referenced, which only works correctly in single-threaded
process."

>> C99 statement sounds stricter:
>> "The value of errno is zero at program startup, but is never set to zero
>> by any library function. 176)"
>> And syscall is not different from library function from C99 point of view.
> Point me to a single line in C99 which mentions ptrace().
> 
> Do you understand what did the commit changed, and what it did not ?
> Setting errno to zero before the syscall was the existing behaviour
> before the change, and I did not modified anything there. But previous
> wrapper set errno to zero in main thread even if called from some other
> thread, which was the bug fixed.

If you may notice, I don't blame you and don't say that you introduce
setting errno to 0. I just want to bring your attention to the problem
while you are in that area and familiar with it.


___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 4:04, Andrey Chernov wrote:
> On 28.08.2016 3:56, Konstantin Belousov wrote:
>> On Sun, Aug 28, 2016 at 03:38:10AM +0300, Andrey Chernov wrote:
>>> On 28.08.2016 2:03, Konstantin Belousov wrote:
   Since ptrace(2) syscall can return -1 for non-error situations, libc
   wrappers set errno to 0 before performing the syscall, as the service
   to the caller.
>>>
>>> Both C99 and POSIX directly prohibits any standard function to set errno
>>> to 0. ptrace() should either choose other errno to indicate non-error
>>> situation or change return -1 to something else.
>>>
>> ptrace(2) is not a standard function.
>>
> 
> C99 statement sounds stricter:
> "The value of errno is zero at program startup, but is never set to zero
> by any library function. 176)"
> And syscall is not different from library function from C99 point of view.
> 

>> And, we cannot break ABI for the syscall.

We can fix already broken (from standards point of view) ABI for the
syscall.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Sun, Aug 28, 2016 at 03:50:04AM +0300, Andrey Chernov wrote:
> On 28.08.2016 3:38, Andrey Chernov wrote:
> > On 28.08.2016 2:03, Konstantin Belousov wrote:
> >>   Since ptrace(2) syscall can return -1 for non-error situations, libc
> >>   wrappers set errno to 0 before performing the syscall, as the service
> >>   to the caller.
> > 
> > Both C99 and POSIX directly prohibits any standard function to set errno
> > to 0. ptrace() should either choose other errno to indicate non-error
> > situation or change return -1 to something else.
> > 
> ...and don't touch errno.
> 
> POSIX: "No function in this volume of POSIX.1-2008 shall set errno to zero."
I am quite curious where ptrace(2) is defined by POSIX.

> 
> > On both i386 and amd64, the errno symbol was directly
> > referenced, which only works correctly in single-threaded process.
> 
> POSIX: "For each thread of a process, the value of errno shall not be
> affected by function calls or assignments to errno by other threads."
And ?  What should the citation add new to the substance
of the code change ?

On Sun, Aug 28, 2016 at 04:04:00AM +0300, Andrey Chernov wrote:
> On 28.08.2016 3:56, Konstantin Belousov wrote:
> > On Sun, Aug 28, 2016 at 03:38:10AM +0300, Andrey Chernov wrote:
> >> On 28.08.2016 2:03, Konstantin Belousov wrote:
> >>>   Since ptrace(2) syscall can return -1 for non-error situations, libc
> >>>   wrappers set errno to 0 before performing the syscall, as the service
> >>>   to the caller.
> >>
> >> Both C99 and POSIX directly prohibits any standard function to set errno
> >> to 0. ptrace() should either choose other errno to indicate non-error
> >> situation or change return -1 to something else.
> >>
> > ptrace(2) is not a standard function.
> > And, we cannot break ABI for the syscall.
> > 
> 
> C99 statement sounds stricter:
> "The value of errno is zero at program startup, but is never set to zero
> by any library function. 176)"
> And syscall is not different from library function from C99 point of view.
Point me to a single line in C99 which mentions ptrace().

Do you understand what did the commit changed, and what it did not ?
Setting errno to zero before the syscall was the existing behaviour
before the change, and I did not modified anything there. But previous
wrapper set errno to zero in main thread even if called from some other
thread, which was the bug fixed.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 3:56, Konstantin Belousov wrote:
> On Sun, Aug 28, 2016 at 03:38:10AM +0300, Andrey Chernov wrote:
>> On 28.08.2016 2:03, Konstantin Belousov wrote:
>>>   Since ptrace(2) syscall can return -1 for non-error situations, libc
>>>   wrappers set errno to 0 before performing the syscall, as the service
>>>   to the caller.
>>
>> Both C99 and POSIX directly prohibits any standard function to set errno
>> to 0. ptrace() should either choose other errno to indicate non-error
>> situation or change return -1 to something else.
>>
> ptrace(2) is not a standard function.
> And, we cannot break ABI for the syscall.
> 

C99 statement sounds stricter:
"The value of errno is zero at program startup, but is never set to zero
by any library function. 176)"
And syscall is not different from library function from C99 point of view.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Sun, Aug 28, 2016 at 03:38:10AM +0300, Andrey Chernov wrote:
> On 28.08.2016 2:03, Konstantin Belousov wrote:
> >   Since ptrace(2) syscall can return -1 for non-error situations, libc
> >   wrappers set errno to 0 before performing the syscall, as the service
> >   to the caller.
> 
> Both C99 and POSIX directly prohibits any standard function to set errno
> to 0. ptrace() should either choose other errno to indicate non-error
> situation or change return -1 to something else.
> 
ptrace(2) is not a standard function.
And, we cannot break ABI for the syscall.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 3:38, Andrey Chernov wrote:
> On 28.08.2016 2:03, Konstantin Belousov wrote:
>>   Since ptrace(2) syscall can return -1 for non-error situations, libc
>>   wrappers set errno to 0 before performing the syscall, as the service
>>   to the caller.
> 
> Both C99 and POSIX directly prohibits any standard function to set errno
> to 0. ptrace() should either choose other errno to indicate non-error
> situation or change return -1 to something else.
> 
...and don't touch errno.

POSIX: "No function in this volume of POSIX.1-2008 shall set errno to zero."

> On both i386 and amd64, the errno symbol was directly
> referenced, which only works correctly in single-threaded process.

POSIX: "For each thread of a process, the value of errno shall not be
affected by function calls or assignments to errno by other threads."
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Andrey Chernov]

On 28.08.2016 2:03, Konstantin Belousov wrote:
>   Since ptrace(2) syscall can return -1 for non-error situations, libc
>   wrappers set errno to 0 before performing the syscall, as the service
>   to the caller.

Both C99 and POSIX directly prohibits any standard function to set errno
to 0. ptrace() should either choose other errno to indicate non-error
situation or change return -1 to something else.

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

Author: kib
Date: Sat Aug 27 23:03:23 2016
New Revision: 304928
URL: https://svnweb.freebsd.org/changeset/base/304928

Log:
  Do not obliterate errno value in the main thread during ptrace(2) call on x86.
  
  Since ptrace(2) syscall can return -1 for non-error situations, libc
  wrappers set errno to 0 before performing the syscall, as the service
  to the caller.  On both i386 and amd64, the errno symbol was directly
  referenced, which only works correctly in single-threaded process.
  
  Change assembler wrappers for ptrace(2) to get current thread errno
  location by calling __error().  Allow __error interposing, as
  currently allowed in cerror().
  
  Sponsored by: The FreeBSD Foundation
  MFC after:1 week

Modified:
  head/lib/libc/amd64/sys/ptrace.S
  head/lib/libc/i386/sys/ptrace.S
  head/lib/libc/sys/ptrace.2

Modified: head/lib/libc/amd64/sys/ptrace.S
==
--- head/lib/libc/amd64/sys/ptrace.SSat Aug 27 22:43:41 2016
(r304927)
+++ head/lib/libc/amd64/sys/ptrace.SSat Aug 27 23:03:23 2016
(r304928)
@@ -38,14 +38,16 @@ __FBSDID("$FreeBSD$");
 
 #include "SYS.h"
 
+   .globl  CNAME(__error)
+   .type   CNAME(__error),@function
+
 ENTRY(ptrace)
-   xorl%eax,%eax
 #ifdef PIC
-   movqPIC_GOT(CNAME(errno)),%r8
-   movl%eax,(%r8)
+   callq   PIC_PLT(CNAME(__error))
 #else
-   movl%eax,CNAME(errno)(%rip)
+   callq   CNAME(__error)
 #endif
+   movl$0,(%rax)
mov $SYS_ptrace,%eax
KERNCALL
jb  HIDENAME(cerror)

Modified: head/lib/libc/i386/sys/ptrace.S
==
--- head/lib/libc/i386/sys/ptrace.S Sat Aug 27 22:43:41 2016
(r304927)
+++ head/lib/libc/i386/sys/ptrace.S Sat Aug 27 23:03:23 2016
(r304928)
@@ -38,16 +38,18 @@ __FBSDID("$FreeBSD$");
 
 #include "SYS.h"
 
+   .globl  CNAME(__error)
+   .type   CNAME(__error),@function
+
 ENTRY(ptrace)
-   xorl%eax,%eax
 #ifdef PIC
-PIC_PROLOGUE
-movlPIC_GOT(CNAME(errno)),%edx
-movl%eax,(%edx)
-PIC_EPILOGUE
+   PIC_PROLOGUE
+   callPIC_PLT(CNAME(__error))
+   PIC_EPILOGUE
 #else
-movl%eax,CNAME(errno)
+   callCNAME(__error)
 #endif
+   movl$0,(%eax)
mov $SYS_ptrace,%eax
KERNCALL
jb  HIDENAME(cerror)

Modified: head/lib/libc/sys/ptrace.2
==
--- head/lib/libc/sys/ptrace.2  Sat Aug 27 22:43:41 2016(r304927)
+++ head/lib/libc/sys/ptrace.2  Sat Aug 27 23:03:23 2016(r304928)
@@ -2,7 +2,7 @@
 .\"$NetBSD: ptrace.2,v 1.2 1995/02/27 12:35:37 cgd Exp $
 .\"
 .\" This file is in the public domain.
-.Dd July 28, 2016
+.Dd August 28, 2016
 .Dt PTRACE 2
 .Os
 .Sh NAME
@@ -906,7 +906,13 @@ to return
 \-1
 as a non-error value; to disambiguate,
 .Va errno
-can be set to 0 before the call and checked afterwards.
+is set to 0 in the libc wrapper for the
+.Fn ptrace
+system call and
+.Fn ptrace
+callers can reliably check
+.Va errno
+for non-zero value afterwards.
 .Sh ERRORS
 The
 .Fn ptrace
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"