[PATCH] Improve ptrace-error detection on Linux targets

classic Classic list List threaded Threaded
99 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/6] Fix comment for 'gdb_dlopen'

Sourceware - gdb-patches mailing list
On Wed, Feb 26, 2020 at 2:06 PM Sergio Durigan Junior
<[hidden email]> wrote:
>
> The 'gdb_dlopen' function doesn't return NULL if the shlib load
> fails;n it actually throws an error.  This patch updates the comment

Typo here (;n)

> to reflect this.
>
> gdbsupport/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>
>         * gdb-dlfcn.h (gdb_dlopen): Update comment.
> ---
>  gdbsupport/gdb-dlfcn.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdbsupport/gdb-dlfcn.h b/gdbsupport/gdb-dlfcn.h
> index 258cfebbc3..9e72a53dc0 100644
> --- a/gdbsupport/gdb-dlfcn.h
> +++ b/gdbsupport/gdb-dlfcn.h
> @@ -32,8 +32,8 @@ struct dlclose_deleter
>  typedef std::unique_ptr<void, dlclose_deleter> gdb_dlhandle_up;
>
>  /* Load the dynamic library file named FILENAME, and return a handle
> -   for that dynamic library.  Return NULL if the loading fails for any
> -   reason.  */
> +   for that dynamic library.  Throw an error if the loading fails for
> +   any reason.  */
>
>  gdb_dlhandle_up gdb_dlopen (const char *filename);
>
> --
> 2.24.1
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/6] Fix comment for 'gdb_dlopen'

Sergio Durigan Junior
On Wednesday, February 26 2020, Christian Biesinger wrote:

> On Wed, Feb 26, 2020 at 2:06 PM Sergio Durigan Junior
> <[hidden email]> wrote:
>>
>> The 'gdb_dlopen' function doesn't return NULL if the shlib load
>> fails;n it actually throws an error.  This patch updates the comment
>
> Typo here (;n)

Thanks!  I fixed this locally.  After I submitted the series, I thought
that this one should probably have been merged with 5/6.  Oh, well...

Cheers,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/6] Document Linux-specific possible ptrace restrictions

Ruslan Kabatsayev
In reply to this post by Sergio Durigan Junior
On Wed, 26 Feb 2020 at 23:06, Sergio Durigan Junior <[hidden email]> wrote:

>
> This patch creates a new "Linux kernel ptrace restrictions" which
> documents possible causes that can be prevent the inferior from being
> correctly started/debugged.
>
> This has been pre-approved by Eli.
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>
>         * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
>         section.
> ---
>  gdb/doc/gdb.texinfo | 143 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 insertions(+)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f1798e35b5..a95158d5d3 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -182,6 +182,9 @@ software in general.  We will miss him.
>                                  @value{GDBN}
>  * Operating System Information:: Getting additional information from
>                                   the operating system
> +* Linux kernel ptrace restrictions::        Restrictions sometimes
> +                                            imposed by the Linux
> +                                            kernel on @code{ptrace}
>  * Trace File Format::          GDB trace file format
>  * Index Section Format::        .gdb_index section format
>  * Man Pages::                  Manual pages
> @@ -45629,6 +45632,146 @@ should contain a comma-separated list of cores that this process
>  is running on.  Target may provide additional columns,
>  which @value{GDBN} currently ignores.
>
> +@node Linux kernel ptrace restrictions
> +@appendix Linux kernel @code{ptrace} restrictions
> +@cindex linux kernel ptrace restrictions, attach
> +
> +The @code{ptrace} system call is used by @value{GDBN} and
> +@code{gdbserver} on GNU/Linux to, among other things, attach to a new
> +or existing inferior in order to start debugging it.  Due to security
> +concerns, some distributions and vendors disable or severely restrict
> +the ability to perform these operations, which can make @value{GDBN}
> +or @code{gdbserver} malfunction.  In this section, we will expand on
> +how this malfunction can manifest itself, and how to modify the
> +system's settings in order to be able to use @value{GDBN} and
> +@code{gdbserver} properly.
> +
> +@menu
> +* The error message::                   The error message displayed when the
> +                                        system prevents @value{GDBN}
> +                                        or @code{gdbserver} from using
> +                                        @code{ptrace}
> +* SELinux's deny_ptrace::               SELinux and the @code{deny_ptrace} option
> +* Yama's ptrace_scope::                 Yama and the @code{ptrace_scope} setting
> +* Docker and seccomp::                  Docker and the @code{seccomp}
> +                                        infrastructure
> +@end menu
> +
> +@node The error message
> +@appendixsection The error message
> +
> +When the system prevents @value{GDBN} or @code{gdbserver} from using
> +the @code{ptrace} system call, you will likely see a descriptive error
> +message explaining what is wrong and how to attempt to fix the
> +problem.  For example, when SELinux's @code{deny_ptrace} option is
> +enabled, you can see:
> +
> +@smallexample
> +$ gdb program
> +...
> +(@value{GDBP}) run
> +Starting program: program
> +warning: Could not trace the inferior process.
> +Error:
> +warning: ptrace: Permission denied
> +The SELinux 'deny_ptrace' option is enabled and preventing @value{GDBN}
> +from using 'ptrace'.  You can disable it by executing (as root):
> +
> +  setsebool deny_ptrace off
> +
> +If you are debugging the inferior remotely, the instruction(s) above must
> +be performed in the target system (e.g., where GDBserver is running).
> +During startup program exited with code 127.
> +(@value{GDBP})
> +@end smallexample
> +
> +Sometimes, it may not be possible to acquire the necessary data to
> +determine the root cause of the failure.  In this case, you will see a
> +generic error message pointing you to this section:
> +
> +@smallexample
> +$ gdb program
> +...
> +Starting program: program
> +warning: Could not trace the inferior process.
> +Error:
> +warning: ptrace: Permission denied
> +There might be restrictions preventing ptrace from working.  Please see
> +the appendix "Linux kernel ptrace restrictions" in the GDB documentation
> +for more details.
> +During startup program exited with code 127.
> +(@value{GDBP})
> +@end smallexample
> +
> +@node SELinux's deny_ptrace
> +@appendixsection SELinux's @code{deny_ptrace}
> +@cindex SELinux
> +@cindex deny_ptrace
> +
> +If you are using SELinux, you might want to check whether the
> +@code{deny_ptrace} option is enabled by doing:
> +
> +@smallexample
> +$ getsebool deny_ptrace
> +deny_ptrace --> on
> +@end smallexample
> +
> +If the option is enabled, you can disable it by doing, as root:
> +
> +@smallexample
> +# setsebool deny_ptrace off
> +@end smallexample
> +
> +The option will be disabled until the next reboot.  If you would like
> +to disable it permanently, you can do (as root):
> +
> +@smallexample
> +# setsebool -P deny_ptrace off
> +@end smallexample
> +
> +@node Yama's ptrace_scope
> +@appendixsection Yama's @code{ptrace_scope}
> +@cindex yama, ptrace_scope
> +
> +If your system has Yama enabled, you might want to check whether the
> +@code{ptrace_scope} setting is enabled by checking the value of
> +@file{/proc/sys/kernel/yama/ptrace_scope}:
> +
> +@smallexample
> +$ cat /proc/sys/kernel/yama/ptrace_scope
> +0
> +@end smallexample
> +
> +If you see anything other than @code{0}, @value{GDBN} or
> +@code{gdbserver} can be affected by it.  You can temporarily disable
> +the feature by doing, as root:
> +
> +@smallexample
> +# sysctl kernel.yama.ptrace_scope=0
> +kernel.yama.ptrace_scope = 0
> +@end smallexample
> +
> +You can make this permanent by doing, as root:
> +
> +@smallexample
> +# sysctl -w kernel.yama.ptrace_scope=0
> +kernel.yama.ptrace_scope = 0
> +@end smallexample

Actually, sysctl's "-w" option doesn't make the setting permanent. It
just lets one write the value. sysctl(8) says about the
"variable=value" syntax:
 > This requires the -w parameter to use.
Though I've found that omitting "-w" works exactly the same on
procps-ng 3.3.9 — checked with strace, which gives identical output in
both cases.
In any case, to make this permanent, one has to modify /etc/sysctl.*
locations (namely, on Ubuntu 14.04 it's /etc/sysctl.d/10-ptrace.conf
).

> +
> +@node Docker and seccomp
> +@appendixsection Docker and @code{seccomp}
> +@cindex docker, seccomp
> +
> +If you are using Docker (@uref{https://www.docker.com/}) containers,
> +you will probably have to disable its @code{seccomp} protections in
> +order to be able to use @value{GDBN} or @code{gdbserver}.  To do that,
> +you can use the options @code{--cap-add=SYS_PTRACE --security-opt
> +seccomp=unconfined} when invoking Docker:
> +
> +@smallexample
> +$ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined
> +@end smallexample
> +
>  @node Trace File Format
>  @appendix Trace File Format
>  @cindex trace file format
> --
> 2.24.1
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/6] Document Linux-specific possible ptrace restrictions

Sergio Durigan Junior
On Wednesday, February 26 2020, Ruslan Kabatsayev wrote:

> On Wed, 26 Feb 2020 at 23:06, Sergio Durigan Junior <[hidden email]> wrote:
>>
>> This patch creates a new "Linux kernel ptrace restrictions" which
>> documents possible causes that can be prevent the inferior from being
>> correctly started/debugged.
>>
>> This has been pre-approved by Eli.
>>
>> gdb/doc/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>>
>>         * gdb.texinfo (Linux kernel ptrace restrictions): New appendix
>>         section.
>> ---
>>  gdb/doc/gdb.texinfo | 143 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 143 insertions(+)
>>
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index f1798e35b5..a95158d5d3 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -182,6 +182,9 @@ software in general.  We will miss him.
>>                                  @value{GDBN}
>>  * Operating System Information:: Getting additional information from
>>                                   the operating system
>> +* Linux kernel ptrace restrictions::        Restrictions sometimes
>> +                                            imposed by the Linux
>> +                                            kernel on @code{ptrace}
>>  * Trace File Format::          GDB trace file format
>>  * Index Section Format::        .gdb_index section format
>>  * Man Pages::                  Manual pages
>> @@ -45629,6 +45632,146 @@ should contain a comma-separated list of cores that this process
>>  is running on.  Target may provide additional columns,
>>  which @value{GDBN} currently ignores.
>>
>> +@node Linux kernel ptrace restrictions
>> +@appendix Linux kernel @code{ptrace} restrictions
>> +@cindex linux kernel ptrace restrictions, attach
>> +
>> +The @code{ptrace} system call is used by @value{GDBN} and
>> +@code{gdbserver} on GNU/Linux to, among other things, attach to a new
>> +or existing inferior in order to start debugging it.  Due to security
>> +concerns, some distributions and vendors disable or severely restrict
>> +the ability to perform these operations, which can make @value{GDBN}
>> +or @code{gdbserver} malfunction.  In this section, we will expand on
>> +how this malfunction can manifest itself, and how to modify the
>> +system's settings in order to be able to use @value{GDBN} and
>> +@code{gdbserver} properly.
>> +
>> +@menu
>> +* The error message::                   The error message displayed when the
>> +                                        system prevents @value{GDBN}
>> +                                        or @code{gdbserver} from using
>> +                                        @code{ptrace}
>> +* SELinux's deny_ptrace::               SELinux and the @code{deny_ptrace} option
>> +* Yama's ptrace_scope::                 Yama and the @code{ptrace_scope} setting
>> +* Docker and seccomp::                  Docker and the @code{seccomp}
>> +                                        infrastructure
>> +@end menu
>> +
>> +@node The error message
>> +@appendixsection The error message
>> +
>> +When the system prevents @value{GDBN} or @code{gdbserver} from using
>> +the @code{ptrace} system call, you will likely see a descriptive error
>> +message explaining what is wrong and how to attempt to fix the
>> +problem.  For example, when SELinux's @code{deny_ptrace} option is
>> +enabled, you can see:
>> +
>> +@smallexample
>> +$ gdb program
>> +...
>> +(@value{GDBP}) run
>> +Starting program: program
>> +warning: Could not trace the inferior process.
>> +Error:
>> +warning: ptrace: Permission denied
>> +The SELinux 'deny_ptrace' option is enabled and preventing @value{GDBN}
>> +from using 'ptrace'.  You can disable it by executing (as root):
>> +
>> +  setsebool deny_ptrace off
>> +
>> +If you are debugging the inferior remotely, the instruction(s) above must
>> +be performed in the target system (e.g., where GDBserver is running).
>> +During startup program exited with code 127.
>> +(@value{GDBP})
>> +@end smallexample
>> +
>> +Sometimes, it may not be possible to acquire the necessary data to
>> +determine the root cause of the failure.  In this case, you will see a
>> +generic error message pointing you to this section:
>> +
>> +@smallexample
>> +$ gdb program
>> +...
>> +Starting program: program
>> +warning: Could not trace the inferior process.
>> +Error:
>> +warning: ptrace: Permission denied
>> +There might be restrictions preventing ptrace from working.  Please see
>> +the appendix "Linux kernel ptrace restrictions" in the GDB documentation
>> +for more details.
>> +During startup program exited with code 127.
>> +(@value{GDBP})
>> +@end smallexample
>> +
>> +@node SELinux's deny_ptrace
>> +@appendixsection SELinux's @code{deny_ptrace}
>> +@cindex SELinux
>> +@cindex deny_ptrace
>> +
>> +If you are using SELinux, you might want to check whether the
>> +@code{deny_ptrace} option is enabled by doing:
>> +
>> +@smallexample
>> +$ getsebool deny_ptrace
>> +deny_ptrace --> on
>> +@end smallexample
>> +
>> +If the option is enabled, you can disable it by doing, as root:
>> +
>> +@smallexample
>> +# setsebool deny_ptrace off
>> +@end smallexample
>> +
>> +The option will be disabled until the next reboot.  If you would like
>> +to disable it permanently, you can do (as root):
>> +
>> +@smallexample
>> +# setsebool -P deny_ptrace off
>> +@end smallexample
>> +
>> +@node Yama's ptrace_scope
>> +@appendixsection Yama's @code{ptrace_scope}
>> +@cindex yama, ptrace_scope
>> +
>> +If your system has Yama enabled, you might want to check whether the
>> +@code{ptrace_scope} setting is enabled by checking the value of
>> +@file{/proc/sys/kernel/yama/ptrace_scope}:
>> +
>> +@smallexample
>> +$ cat /proc/sys/kernel/yama/ptrace_scope
>> +0
>> +@end smallexample
>> +
>> +If you see anything other than @code{0}, @value{GDBN} or
>> +@code{gdbserver} can be affected by it.  You can temporarily disable
>> +the feature by doing, as root:
>> +
>> +@smallexample
>> +# sysctl kernel.yama.ptrace_scope=0
>> +kernel.yama.ptrace_scope = 0
>> +@end smallexample
>> +
>> +You can make this permanent by doing, as root:
>> +
>> +@smallexample
>> +# sysctl -w kernel.yama.ptrace_scope=0
>> +kernel.yama.ptrace_scope = 0
>> +@end smallexample
>
> Actually, sysctl's "-w" option doesn't make the setting permanent. It
> just lets one write the value. sysctl(8) says about the
> "variable=value" syntax:
>  > This requires the -w parameter to use.
> Though I've found that omitting "-w" works exactly the same on
> procps-ng 3.3.9 — checked with strace, which gives identical output in
> both cases.
> In any case, to make this permanent, one has to modify /etc/sysctl.*
> locations (namely, on Ubuntu 14.04 it's /etc/sysctl.d/10-ptrace.conf
> ).

Thanks.  I'll remove this part from the docs, then.

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/6] Fix comment for 'gdb_dlopen'

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

Sergio> gdbsupport/ChangeLog:
Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

Sergio> * gdb-dlfcn.h (gdb_dlopen): Update comment.

This one could go in immediately under the obvious rule.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

Sergio> gdb/ChangeLog:
Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

Sergio> * unittests/scoped_pipe-selftests.c: New file.

Sergio> gdbsupport/ChangeLog:
Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>

Sergio> * scoped_pipe.h: New file.

I think new general-purpose code should go in gdbsupport.

Unfortunately right now we don't have a good way to put the unit tests
there.  This is something we ought to do eventually though.

Sergio> +#ifndef COMMON_SCOPED_PIPE_H

I think we've been using GDBSUPPORT rather than COMMON now.

Sergio> +  explicit scoped_pipe ()

I think explicit is only needed for 1-arg constructors.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

Sergio> Since this hunk may be a bit controversial, I decided to split it into
Sergio> a separate patch.  This is going to be needed by the ptrace-error
Sergio> feature; GDB will need to be able to access the value of errno even
Sergio> after a call to our 'perror'-like functions.

I'm in favor of this.  The existing code seems pretty ugly.

I'd imagine it's unlikely that any caller would rely on this.
If it tested cleanly then that is good enough for me.

Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
Sergio> 'perror_with_name', but this one is pretty trivial, I think.

I didn't understand why this one was needed.
Does safe_strerror reset errno?  Maybe a comment would be in order.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/6] Fix comment for 'gdb_dlopen'

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Friday, February 28 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> gdbsupport/ChangeLog:
> Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>
> Sergio> * gdb-dlfcn.h (gdb_dlopen): Update comment.
>
> This one could go in immediately under the obvious rule.

Thanks, Tom.

Pushed: d7592e974706637058867b02626c52a30ef0a2ee

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Friday, February 28 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> gdb/ChangeLog:
> Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>
> Sergio> * unittests/scoped_pipe-selftests.c: New file.
>
> Sergio> gdbsupport/ChangeLog:
> Sergio> yyyy-mm-dd  Sergio Durigan Junior  <[hidden email]>
>
> Sergio> * scoped_pipe.h: New file.
>
> I think new general-purpose code should go in gdbsupport.

Yep.  scoped_pipe.h is there.

> Unfortunately right now we don't have a good way to put the unit tests
> there.  This is something we ought to do eventually though.

Right, that's something I noticed as well.

> Sergio> +#ifndef COMMON_SCOPED_PIPE_H
>
> I think we've been using GDBSUPPORT rather than COMMON now.

Ah, OK.  I found other places using COMMON, but I'll change it here.

> Sergio> +  explicit scoped_pipe ()
>
> I think explicit is only needed for 1-arg constructors.

OK, I'll remove the keyword.

I'll make the changes locally.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Friday, February 28 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> Since this hunk may be a bit controversial, I decided to split it into
> Sergio> a separate patch.  This is going to be needed by the ptrace-error
> Sergio> feature; GDB will need to be able to access the value of errno even
> Sergio> after a call to our 'perror'-like functions.
>
> I'm in favor of this.  The existing code seems pretty ugly.
>
> I'd imagine it's unlikely that any caller would rely on this.
> If it tested cleanly then that is good enough for me.

As far as I have tested (buildbot and locally), everything is OK.

> Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
> Sergio> 'perror_with_name', but this one is pretty trivial, I think.
>
> I didn't understand why this one was needed.
> Does safe_strerror reset errno?  Maybe a comment would be in order.

Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
message, it is not allowed to do so.  The reason I decided to explicitly
save/restore errno is because I wanted to make sure that we (also
explicitly) follow the same rules for both GDB and gdbserver perror
functions.  But now I'm left thinking that I should have proposed the
same thing for 'throw_perror_with_name'...

Anyway, I'm fine with removing the save/restore from gdbserver's
perror.  I'll also remove the reference to it in the commit message.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>> I think new general-purpose code should go in gdbsupport.

Sergio> Yep.  scoped_pipe.h is there.

Lol, sorry about that.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Tom Tromey-2
In reply to this post by Sergio Durigan Junior
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:

>> I didn't understand why this one was needed.
>> Does safe_strerror reset errno?  Maybe a comment would be in order.

Sergio> Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
Sergio> message, it is not allowed to do so.  The reason I decided to explicitly
Sergio> save/restore errno is because I wanted to make sure that we (also
Sergio> explicitly) follow the same rules for both GDB and gdbserver perror
Sergio> functions.  But now I'm left thinking that I should have proposed the
Sergio> same thing for 'throw_perror_with_name'...

Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's
Sergio> perror.  I'll also remove the reference to it in the commit message.

The main thing for me is that when someone sees this in 5 years, that
there's some idea of why the line is there.  Leaving the line out is
just as good :)

thanks,
Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Pedro Alves-7
In reply to this post by Sergio Durigan Junior
On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:

> --- /dev/null
> +++ b/gdb/unittests/scoped_pipe-selftests.c
> @@ -0,0 +1,96 @@

> +
> +#include "defs.h"
> +
> +#include "gdbsupport/scoped_pipe.h"
> +#include "config.h"

Was there a reason for this "config.h" include?

Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Sergio Durigan Junior
On Friday, February 28 2020, Pedro Alves wrote:

> On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:
>
>> --- /dev/null
>> +++ b/gdb/unittests/scoped_pipe-selftests.c
>> @@ -0,0 +1,96 @@
>
>> +
>> +#include "defs.h"
>> +
>> +#include "gdbsupport/scoped_pipe.h"
>> +#include "config.h"
>
> Was there a reason for this "config.h" include?

I copied this test from the existing scoped_fd-selftests.c and adjusted
it.  I did remove other includes, but left this one untouched.  I tried
removing it now, and the file builds just fine, so I'll update my copy
of the patch here.  Thanks!

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Friday, February 28 2020, Tom Tromey wrote:

>>> I think new general-purpose code should go in gdbsupport.
>
> Sergio> Yep.  scoped_pipe.h is there.
>
> Lol, sorry about that.

That's alright!  I was confused with your comment, but wasn't sure if
you meant something else by it ;-).

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Pedro Alves-7
In reply to this post by Sergio Durigan Junior
On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:

>
> git blame tells me that this piece of code is pretty old; the commit
> that "introduced" it is:
>
>   commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
>   Author: Stan Shebs <[hidden email]>
>   Date:   Fri Apr 16 01:35:26 1999 +0000
>
>       Initial creation of sourceware repository
>
> so yeah...

This is not the oldest commit in the tree.  Using git log
starting at the hash, you should be able to find older commits
which touch the file.  The thing is that history around the time
of that "initial creation" commit, is quite messy, because the
CVS tree back then was deleted and recreated a number of times
and merged in some odd ways.

Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
On Friday, February 28 2020, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
>>> I didn't understand why this one was needed.
>>> Does safe_strerror reset errno?  Maybe a comment would be in order.
>
> Sergio> Ah, no, safe_strerror doesn't reset errno.  As I explained in the commit
> Sergio> message, it is not allowed to do so.  The reason I decided to explicitly
> Sergio> save/restore errno is because I wanted to make sure that we (also
> Sergio> explicitly) follow the same rules for both GDB and gdbserver perror
> Sergio> functions.  But now I'm left thinking that I should have proposed the
> Sergio> same thing for 'throw_perror_with_name'...
>
> Sergio> Anyway, I'm fine with removing the save/restore from gdbserver's
> Sergio> perror.  I'll also remove the reference to it in the commit message.
>
> The main thing for me is that when someone sees this in 5 years, that
> there's some idea of why the line is there.  Leaving the line out is
> just as good :)

Right, that's an excellent argument.  It's safe to leave the line out
right now; the important thing is that we don't touch errno, anyway.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Sergio Durigan Junior
In reply to this post by Pedro Alves-7
On Friday, February 28 2020, Pedro Alves wrote:

> On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:
>>
>> git blame tells me that this piece of code is pretty old; the commit
>> that "introduced" it is:
>>
>>   commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc
>>   Author: Stan Shebs <[hidden email]>
>>   Date:   Fri Apr 16 01:35:26 1999 +0000
>>
>>       Initial creation of sourceware repository
>>
>> so yeah...
>
> This is not the oldest commit in the tree.  Using git log
> starting at the hash, you should be able to find older commits
> which touch the file.  The thing is that history around the time
> of that "initial creation" commit, is quite messy, because the
> CVS tree back then was deleted and recreated a number of times
> and merged in some odd ways.

Sorry, I didn't mean to say that this is the oldest commit in the tree.
But even doing a bit more search, I could only find this commit:

  commit 8eec331072987d38064745a33ae89cc5d195029c
  Author: Steve Chamberlain <sac@cygnus>
  Date:   Sat Mar 19 03:16:10 1994 +0000

              * utils.c (prompt_for_continue): Call readline, not gdb_readline.

which did:

  -  bfd_error = no_error;
  +  bfd_set_error (bfd_error_no_error);

This means the code is really old (pre-1994), and I could not track when
it was added.  And I don't think it matters much at this point: even if
I could find the exact commit that added it, I doubt I'd be able to find
the rationale.

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/6] Don't reset errno/bfd_error on 'throw_perror_with_name'

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 2/28/20 3:29 PM, Tom Tromey wrote:
>>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> writes:
>
> Sergio> Since this hunk may be a bit controversial, I decided to split it into
> Sergio> a separate patch.  This is going to be needed by the ptrace-error
> Sergio> feature; GDB will need to be able to access the value of errno even
> Sergio> after a call to our 'perror'-like functions.
>
> I'm in favor of this.  The existing code seems pretty ugly.

I'm not sure in favor of relying on errno being preserved from
throw site to catch site, with potentially multiple try/catch hops
in between.  Sergio, can you point out exactly how you're
intending to use that?

But, I'm in favor of removing this errno/bfd_error clearing too.

We used to have more global state clearing around this area,
but it's been gradually removed.  E.g.:

commit 0af679c6e0645a93d5a60ec936b94dc70a2f9e5c
Author:     Pedro Alves <[hidden email]>
AuthorDate: Tue Apr 12 16:49:30 2016 +0100
Commit:     Pedro Alves <[hidden email]>
CommitDate: Tue Apr 12 16:55:35 2016 +0100

    Don't call clear_quit_flag in prepare_to_throw_exception
   
    I think this is reminiscent of the time when a longjmp would always
    jump to the top level.  Nowaways code that throw exceptions other than
    a quit, which may even be caught and handled without reaching the top
    level.  Certainly such exceptions shouldn't clear an interrupt
    request...


I suspect this perror_with_name errno/bfd_error clearing was also
related to ancient direct longjmp to the top level.

> I'd imagine it's unlikely that any caller would rely on this.
> If it tested cleanly then that is good enough for me.




>
> Sergio> Another small hunk is the one that saves/restores errno on gdbserver's
> Sergio> 'perror_with_name', but this one is pretty trivial, I think.
>
> I didn't understand why this one was needed.
> Does safe_strerror reset errno?  Maybe a comment would be in order.
Thanks,
Pedro Alves

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/6] Introduce scoped_pipe.h

Pedro Alves-7
In reply to this post by Sergio Durigan Junior
On 2/28/20 7:47 PM, Sergio Durigan Junior wrote:
> On Friday, February 28 2020, Pedro Alves wrote:
>
>> On 2/26/20 8:05 PM, Sergio Durigan Junior wrote:
>>

>>> --- /dev/null
>>> +++ b/gdb/unittests/scoped_pipe-selftests.c
>>> @@ -0,0 +1,96 @@
>>
>>> +
>>> +#include "defs.h"
>>> +
>>> +#include "gdbsupport/scoped_pipe.h"
>>> +#include "config.h"
>>
>> Was there a reason for this "config.h" include?
>
> I copied this test from the existing scoped_fd-selftests.c and adjusted
> it.  I did remove other includes, but left this one untouched.  I tried
> removing it now, and the file builds just fine, so I'll update my copy
> of the patch here.  Thanks!

Ah, I see.  That config.h include shouldn't be there in
scoped_fd-selftests.c either.

Thanks,
Pedro Alves

12345