Message ID | 1534932677-9496-4-git-send-email-roirand@adacore.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 60397 invoked by alias); 22 Aug 2018 10:11:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 60287 invoked by uid 89); 22 Aug 2018 10:11:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_NEUTRAL autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Aug 2018 10:11:31 +0000 Received: by mail-wr1-f65.google.com with SMTP id v17-v6so1134816wrr.9 for <gdb-patches@sourceware.org>; Wed, 22 Aug 2018 03:11:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=mznWm0+QwoEhRY6sKb1YxIg3iIyVHWb6G7hDbLl7ZvA=; b=FmuQJ+PyFjEnekDF+OFWrUQvbmsDdgfs6wZQyUuw3VHPBx/u+903TItjEVw6HlkAcI w/GNPFo2R8Bwu9iTvZk3+NBcK6WoBVW2XE7No/8K13gy/qQg5iWdT1uipa/SGfJE8CpC lIQdOmpVmR5x+VAZ/DK/NcLdX1Ba0GVfB2S6spOpyAj7i9p/RPHNlSX07jJ6hYEMSaB7 BwQqCW6mLqZ0qdkL60JtitpcrH0Aa+YEJE99lE33uGE8G+eFiaaykIrPIDiZ7SJKIRYM YYS/dtT24we8Kzh73GKObRuXLiVKFl2EPx+it6xytxTyWgt9j0pyNwOsOeU0Ea/yCkKz T94A== Return-Path: <roirand@adacore.com> Received: from adacore.com ([46.18.100.10]) by smtp.gmail.com with ESMTPSA id n11-v6sm1280859wra.26.2018.08.22.03.11.27 (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 22 Aug 2018 03:11:28 -0700 (PDT) Received: by adacore.com (sSMTP sendmail emulation); Wed, 22 Aug 2018 12:11:26 +0200 From: Xavier Roirand <roirand@adacore.com> To: gdb-patches@sourceware.org Cc: brobecker@adacore.com, Xavier Roirand <roirand@adacore.com> Subject: [RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later. Date: Wed, 22 Aug 2018 12:11:15 +0200 Message-Id: <1534932677-9496-4-git-send-email-roirand@adacore.com> In-Reply-To: <1534932677-9496-1-git-send-email-roirand@adacore.com> References: <1534932677-9496-1-git-send-email-roirand@adacore.com> X-IsSubscribed: yes |
Commit Message
Xavier Roirand
Aug. 22, 2018, 10:11 a.m. UTC
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(+)
Comments
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
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 () +{ + 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; + } +} + 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 (); }