[RFA,3/5] Darwin: set startup-with-shell to off on Sierra and later.

  On Mac OS X Sierra and later, the shell is not allowed to be
debug so add a check and disable startup with shell in that
case.

gdb/ChangeLog:
    * darwin-nat.c (disable_startup_with_shell): New function.
    (_initialize_darwin_inferior): Add call.

Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
---
 gdb/darwin-nat.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
  

On 2018-08-22 06:11, Xavier Roirand wrote:
> On Mac OS X Sierra and later, the shell is not allowed to be
> debug so add a check and disable startup with shell in that
> case.

Ah, that's a really good idea to do it automatically, instead of asking 
the user to do it.

> gdb/ChangeLog:
>     * darwin-nat.c (disable_startup_with_shell): New function.
>     (_initialize_darwin_inferior): Add call.
> 
> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
> ---
>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index be80163..96f70cf 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>    return true;
>  }
> 
> +/* Read kernel version, and set startup-with-shell to false on Sierra 
> or
> +   later.  */
> +
> +void
> +disable_startup_with_shell ()

This function should be static.

> +{
> +  char str[16];
> +    size_t sz = sizeof (str);
> +    int ret;
> +    unsigned long ver;
> +
> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
> +    if (ret == 0 && sz < sizeof (str))
> +      {
> +       ver = strtoul (str, NULL, 10);
> +       if (ver >= 16)
> +         startup_with_shell = 0;
> +      }
> +}

The indentation is not quite right.  You can also declare variables when 
they are used/initialized.

> +
>  void
>  _initialize_darwin_nat ()
>  {
> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
> are reported before being\n\
>  reported by the kernel."),
>  			   &set_enable_mach_exceptions, NULL,
>  			   &setlist, &showlist);
> +
> +  disable_startup_with_shell ();
>  }

I don't think we should do that in _initialize_darwin_nat.  Since 
startup-with-shell is supported with remote debugging, you could still 
use it when starting a Linux remote program from a macOS host.

Instead, we should probably only disable it at the moment we create a 
new inferior in the darwin_nat target, so in 
darwin_nat_target::create_inferior.  We also don't want to permanently 
change the setting, so we should restore the value.  Presumably, putting 
something like this in darwin_nat_target::create_inferior should work (I 
haven't tested it, and sorry for the potential formatting mess my email 
client will do):

   gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
   if (startup_with_shell && should_disable_startup_with_shell ())
     {
       warning (_("startup-with-shell not supported on this macOS 
version, disabling it."));
       restore_startup_with_shell.emplace (&startup_with_shell, 0);
     }

Instead of setting/restoring startup_with_shell, it might be better if 
its value was passed as a parameter all the way down instead of having 
functions read the global variable, but that's a bigger job.

Simon
  

On 08/22/2018 03:20 PM, Simon Marchi wrote:
> On 2018-08-22 06:11, Xavier Roirand wrote:
>> On Mac OS X Sierra and later, the shell is not allowed to be
>> debug so add a check and disable startup with shell in that
>> case.
> 
> Ah, that's a really good idea to do it automatically, instead of asking the user to do it.

See also:

  https://sourceware.org/ml/gdb-patches/2018-06/msg00734.html

BTW, on IRC, Tromey and I discussed how to make startup-with-shell
work, which led to:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23364

which points at what seems like the simplest solution (copy the shell
to /tmp).  

An alternative would be to use a similar approach to
lldb's -- run a helper tool (lldb-argdumper) via the shell whose only
purpose is to return back its shell-expanded argv[0..N].  That would work
because gdb wouldn't debug that program, just run it normally, so it
wouldn't run into the SIP restrictions.  Instead of a separate helper tool, I would
imagine we could also build the helper functionality inside gdb itself -- we'd
make gdb behave in that "expand args" mode via an environment
variable, for example.

> 
>> gdb/ChangeLog:
>>     * darwin-nat.c (disable_startup_with_shell): New function.
>>     (_initialize_darwin_inferior): Add call.
>>
>> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
>> ---
>>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index be80163..96f70cf 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>>    return true;
>>  }
>>
>> +/* Read kernel version, and set startup-with-shell to false on Sierra or
>> +   later.  */
>> +
>> +void
>> +disable_startup_with_shell ()
> 
> This function should be static.
> 
>> +{
>> +  char str[16];
>> +    size_t sz = sizeof (str);
>> +    int ret;
>> +    unsigned long ver;
>> +
>> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
>> +    if (ret == 0 && sz < sizeof (str))
>> +      {
>> +       ver = strtoul (str, NULL, 10);
>> +       if (ver >= 16)
>> +         startup_with_shell = 0;
>> +      }
>> +}
> 
> The indentation is not quite right.  You can also declare variables when they are used/initialized.
> 
>> +
>>  void
>>  _initialize_darwin_nat ()
>>  {
>> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
>> are reported before being\n\
>>  reported by the kernel."),
>>                 &set_enable_mach_exceptions, NULL,
>>                 &setlist, &showlist);
>> +
>> +  disable_startup_with_shell ();
>>  }
> 
> I don't think we should do that in _initialize_darwin_nat.  Since startup-with-shell is supported with remote debugging, you could still use it when starting a Linux remote program from a macOS host.
> 
> Instead, we should probably only disable it at the moment we create a new inferior in the darwin_nat target, so in darwin_nat_target::create_inferior.  We also don't want to permanently change the setting, so we should restore the value.  Presumably, putting something like this in darwin_nat_target::create_inferior should work (I haven't tested it, and sorry for the potential formatting mess my email client will do):
> 
>   gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
>   if (startup_with_shell && should_disable_startup_with_shell ())
>     {
>       warning (_("startup-with-shell not supported on this macOS version, disabling it."));
>       restore_startup_with_shell.emplace (&startup_with_shell, 0);
>     }
> 
> Instead of setting/restoring startup_with_shell, it might be better if its value was passed as a parameter all the way down instead of having functions read the global variable, but that's a bigger job.
Thanks,
Pedro Alves
  

Hello,

Le 8/22/18 à 4:20 PM, Simon Marchi a écrit :
> On 2018-08-22 06:11, Xavier Roirand wrote:
>> On Mac OS X Sierra and later, the shell is not allowed to be
>> debug so add a check and disable startup with shell in that
>> case.
> 
> Ah, that's a really good idea to do it automatically, instead of asking 
> the user to do it.
> 
>> gdb/ChangeLog:
>>     * darwin-nat.c (disable_startup_with_shell): New function.
>>     (_initialize_darwin_inferior): Add call.
>>
>> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
>> ---
>>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index be80163..96f70cf 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>>    return true;
>>  }
>>
>> +/* Read kernel version, and set startup-with-shell to false on Sierra or
>> +   later.  */
>> +
>> +void
>> +disable_startup_with_shell ()
> 
> This function should be static.
> 
>> +{
>> +  char str[16];
>> +    size_t sz = sizeof (str);
>> +    int ret;
>> +    unsigned long ver;
>> +
>> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
>> +    if (ret == 0 && sz < sizeof (str))
>> +      {
>> +       ver = strtoul (str, NULL, 10);
>> +       if (ver >= 16)
>> +         startup_with_shell = 0;
>> +      }
>> +}
> 
> The indentation is not quite right.  You can also declare variables when 
> they are used/initialized.
> 
>> +
>>  void
>>  _initialize_darwin_nat ()
>>  {
>> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
>> are reported before being\n\
>>  reported by the kernel."),
>>                 &set_enable_mach_exceptions, NULL,
>>                 &setlist, &showlist);
>> +
>> +  disable_startup_with_shell ();
>>  }
> 
> I don't think we should do that in _initialize_darwin_nat.  Since 
> startup-with-shell is supported with remote debugging, you could still 
> use it when starting a Linux remote program from a macOS host.
> 
> Instead, we should probably only disable it at the moment we create a 
> new inferior in the darwin_nat target, so in 
> darwin_nat_target::create_inferior.  We also don't want to permanently 
> change the setting, so we should restore the value.  Presumably, putting 
> something like this in darwin_nat_target::create_inferior should work (I 
> haven't tested it, and sorry for the potential formatting mess my email 
> client will do):
> 
>    gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
>    if (startup_with_shell && should_disable_startup_with_shell ())
>      {
>        warning (_("startup-with-shell not supported on this macOS 
> version, disabling it."));
>        restore_startup_with_shell.emplace (&startup_with_shell, 0);
>      }
> 

Thanks for the remarks, I'll update my patch accordingly.

Regards.

> Instead of setting/restoring startup_with_shell, it might be better if 
> its value was passed as a parameter all the way down instead of having 
> functions read the global variable, but that's a bigger job.
> 
> Simon
  

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> On Mac OS X Sierra and later, the shell is not allowed to be
Xavier> debug so add a check and disable startup with shell in that
Xavier> case.

I learned recently that System Integrity Protection was introduced in
El Capitan:

    https://support.apple.com/en-us/HT204899

So, I'm curious why this patch checks for Sierra.

I thought maybe it could just be a bug, but also that maybe the ptrace
changes were added in Sierra.

Do you know?  I don't have an older machine to try on.  I think it would
be good to document or fix this decision, depending on what's going on.

Tom