Re: [PATCH v2 6/6] Cache a copy of the user's shell on macOS

[Simon Marchi <simark at simark dot ca>]

On 2018-10-18 6:31 p.m., Tom Tromey wrote:
> +/* If $SHELL is restricted, try to cache a copy.  Starting with El
> +   Capitan, macOS introduced System Integrity Protection.  Among other
> +   things, this prevents certain executables from being ptrace'd.  In
> +   particular, executables in /bin, like most shells, are affected.
> +   To work around this, while preserving command-line glob expansion
> +   and redirections, gdb will cache a copy of the shell.  Return true
> +   if all is well -- either the shell is not subject to SIP or it has
> +   been successfully cached.  Returns false if something failed.  */
> +
> +static bool
> +maybe_cache_shell ()
> +{
> +  /* SF_RESTRICTED is defined in sys/stat.h and lets us determine if a
> +     given file is subject to SIP.  */
> +#ifdef SF_RESTRICTED
> +
> +  /* If a check fails we want to revert -- maybe the user deleted the
> +     cache while gdb was running, or something like that.  */
> +  copied_shell = nullptr;
> +
> +  const char *shell = get_shell ();
> +  if (!IS_ABSOLUTE_PATH (shell))
> +    {
> +      warning (_("This version of macOS has System Integrity Protection.\n\
> +Normally gdb would try to work around this by caching a copy of your shell,\n\
> +but because your shell (%s) is not an absolute path, this is being skipped."),
> +	       shell);
> +      return false;
> +    }
> +
> +  struct stat sb;
> +  if (stat (shell, &sb) < 0)
> +    {
> +      warning (_("This version of macOS has System Integrity Protection.\n\
> +Normally gdb would try to work around this by caching a copy of your shell,\n\
> +but because gdb could not stat your shell (%s), this is being skipped.\n\
> +The error was: %s"),
> +	       shell, safe_strerror (errno));
> +      return false;
> +    }
> +
> +  if ((sb.st_flags & SF_RESTRICTED) == 0)
> +    return true;
> +
> +  /* Put the copy somewhere like ~/Library/Caches/gdb/bin/sh.  */
> +  std::string new_name = get_standard_cache_dir ();
> +  if (!IS_DIR_SEPARATOR (new_name.back ()) && !IS_ABSOLUTE_PATH (shell))

I mentioned something about this on the version of the patch, but you might
have missed it since you didn't reply to that specifically, so I'll send
it again just to be sure:

I believe this !IS_ABSOLUTE_PATH check can never be true, since we would
have returned early if it was the case?  If so, this append is not needed

Simon