[RFC 0/6] A different approach to startup-with-shell on macOS

[RFC 0/6] A different approach to startup-with-shell on macOS

[Tom Tromey]

Currently the macOS port will disable startup-with-shell on versions
of macOS that have System Integrity Protection.  This is done because
with SIP, gdb cannot ptrace certain executables, including the normal
shells.

This series implements a different approach: copy the user's shell
executable to the cache directory and arrange to use the copy.  This
avoids the SIP restrictions.

Most of the series is just cleanup, rearranging so some private
functions can be shared, and fixing a few small things I noticed along
the way.

This has been regression tested by one of the buildbot builders, and
then I tested the final patch on macOS High Sierra.

One question I have is whether it's possible to build gdb on an older
version of macOS and then run it on a newer version.  If this can be
done, then the #if-based approach taken in the final patch will not
work.

I didn't include any way to control this feature other than "set
startup-with-shell off".  My thinking was that turning this off will
just result in failures, which isn't useful.  However if there's a
reason to do something else, I could add it.

Tom

[RFC 1/6] Unify shell-finding logic

[Tom Tromey]

I noticed several places in gdb that were using getenv("SHELL") and
then falling back to "/bin/sh" if it returned NULL.  This unifies
these into a single function.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* procfs.c (procfs_target::create_inferior): Use get_shell.
	* cli/cli-cmds.c (shell_escape): Use get_shell.
	* windows-nat.c (windows_nat_target::create_inferior): Use
	get_shell.
	* common/pathstuff.c (get_shell): New function.
	* nat/fork-inferior.c (SHELL_FILE, get_startup_shell): Remove.
	(fork_inferior): Use get_shell.
	* common/pathstuff.h (get_shell): Declare.
---
 gdb/ChangeLog           | 11 +++++++++++
 gdb/cli/cli-cmds.c      |  6 ++----
 gdb/common/pathstuff.c  | 12 ++++++++++++
 gdb/common/pathstuff.h  |  4 ++++
 gdb/nat/fork-inferior.c | 21 ++-------------------
 gdb/procfs.c            |  4 ++--
 gdb/windows-nat.c       |  5 ++---
 7 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index c60e5efd0c..37988dd5d7 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -50,6 +50,7 @@
 #include "cli/cli-utils.h"
 
 #include "extension.h"
+#include "common/pathstuff.h"
 
 #ifdef TUI
 #include "tui/tui.h"	/* For tui_active et.al.  */
@@ -726,13 +727,10 @@ shell_escape (const char *arg, int from_tty)
 
   if ((pid = vfork ()) == 0)
     {
-      const char *p, *user_shell;
+      const char *p, *user_shell = get_shell ();
 
       close_most_fds ();
 
-      if ((user_shell = getenv ("SHELL")) == NULL)
-	user_shell = "/bin/sh";
-
       /* Get the name of the shell for arg0.  */
       p = lbasename (user_shell);
 
diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index 82905c9e68..6d8e53f4e1 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -190,3 +190,15 @@ get_standard_cache_dir ()
 
   return {};
 }
+
+/* See common/pathstuff.h.  */
+
+const char *
+get_shell ()
+{
+  const char *ret = getenv ("SHELL");
+  if (ret == NULL)
+    ret = "/bin/sh";
+
+  return ret;
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
index a43b963651..40fbda8d85 100644
--- a/gdb/common/pathstuff.h
+++ b/gdb/common/pathstuff.h
@@ -64,4 +64,8 @@ extern bool contains_dir_separator (const char *path);
 
 extern std::string get_standard_cache_dir ();
 
+/* Return the file name of the user's shell.  */
+
+extern const char *get_shell ();
+
 #endif /* PATHSTUFF_H */
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 40cd05a0f8..f1032b43c9 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -24,16 +24,13 @@
 #include "target/target.h"
 #include "common-inferior.h"
 #include "common-gdbthread.h"
+#include "common/pathstuff.h"
 #include "signals-state-save-restore.h"
 #include "gdb_tilde_expand.h"
 #include <vector>
 
 extern char **environ;
 
-/* Default shell file to be used if 'startup-with-shell' is set but
-   $SHELL is not.  */
-#define SHELL_FILE "/bin/sh"
-
 /* Build the argument vector for execv(3).  */
 
 class execv_argv
@@ -265,20 +262,6 @@ execv_argv::init_for_shell (const char *exec_file,
   m_argv.push_back (NULL);
 }
 
-/* Return the shell that must be used to startup the inferior.  The
-   first attempt is the environment variable SHELL; if it is not set,
-   then we default to SHELL_FILE.  */
-
-static const char *
-get_startup_shell ()
-{
-  const char *ret = getenv ("SHELL");
-  if (ret == NULL)
-    ret = SHELL_FILE;
-
-  return ret;
-}
-
 /* See nat/fork-inferior.h.  */
 
 pid_t
@@ -316,7 +299,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
 
       /* Figure out what shell to start up the user program under.  */
       if (shell_file == NULL)
-	shell_file = get_startup_shell ();
+	shell_file = get_shell ();
 
       gdb_assert (shell_file != NULL);
     }
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 6ffe569e69..ca381a71ae 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -3035,11 +3035,11 @@ procfs_target::create_inferior (const char *exec_file,
 				const std::string &allargs,
 				char **env, int from_tty)
 {
-  char *shell_file = getenv ("SHELL");
+  const char *shell_file = get_shell ();
   char *tryname;
   int pid;
 
-  if (shell_file != NULL && strchr (shell_file, '/') == NULL)
+  if (strchr (shell_file, '/') == NULL)
     {
 
       /* We will be looking down the PATH to find shell_file.  If we
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 0047a26418..8292cf4212 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -68,6 +68,7 @@
 #include "complaints.h"
 #include "inf-child.h"
 #include "gdb_tilde_expand.h"
+#include "common/pathstuff.h"
 
 #define AdjustTokenPrivileges		dyn_AdjustTokenPrivileges
 #define DebugActiveProcessStop		dyn_DebugActiveProcessStop
@@ -2578,9 +2579,7 @@ windows_nat_target::create_inferior (const char *exec_file,
     }
   else
     {
-      sh = getenv ("SHELL");
-      if (!sh)
-	sh = "/bin/sh";
+      sh = get_shell ();
       if (cygwin_conv_path (CCP_POSIX_TO_WIN_W, sh, shell, __PMAX) < 0)
       	error (_("Error starting executable via shell: %d"), errno);
 #ifdef __USEWIDE
-- 
2.17.1

Re: [RFC 1/6] Unify shell-finding logic

[Simon Marchi]

On 2018-09-26 07:11, Tom Tromey wrote:
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index 6ffe569e69..ca381a71ae 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -3035,11 +3035,11 @@ procfs_target::create_inferior (const char 
> *exec_file,
>  				const std::string &allargs,
>  				char **env, int from_tty)
>  {
> -  char *shell_file = getenv ("SHELL");
> +  const char *shell_file = get_shell ();
>    char *tryname;
>    int pid;
> 
> -  if (shell_file != NULL && strchr (shell_file, '/') == NULL)
> +  if (strchr (shell_file, '/') == NULL)

At first I thought this would change the behavior here, but I think it's 
fine.  If SHELL is not defined, we used to pass NULL to fork_inferior, 
and it would fall back to /bin/sh.  Now, the fallback just happens 
sooner.  So this looks good, I think this patch can go in by itself.

Thanks,

Simon

[RFC 5/6] Do not reopen temporary files

[Tom Tromey]

The current callers of mkostemp close the file descriptor and then
re-open it with fopen.  It seemed better to me to continue to use the
already-opened file descriptor, so this patch rearranges the code a
little in order to do so.  It takes care to ensure that the files are
only unlinked after the file descriptor in question is closed, as
before.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* unittests/scoped_fd-selftests.c (test_to_file): New function.
	(run_tests): Call test_to_file.
	* dwarf-index-write.c (write_psymtabs_to_index): Do not reopen
	temporary files.
	* common/scoped_fd.h (scoped_fd::to_file): New method.
---
 gdb/ChangeLog                       |  8 +++++
 gdb/common/scoped_fd.h              | 13 +++++++
 gdb/dwarf-index-write.c             | 55 ++++++++++++++---------------
 gdb/unittests/scoped_fd-selftests.c | 17 +++++++++
 4 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/gdb/common/scoped_fd.h b/gdb/common/scoped_fd.h
index a6a8ab9a38..f37776ae21 100644
--- a/gdb/common/scoped_fd.h
+++ b/gdb/common/scoped_fd.h
@@ -25,6 +25,7 @@
 #ifdef HAVE_UNISTD_H
 
 #include <unistd.h>
+#include "filestuff.h"
 
 /* A smart-pointer-like class to automatically close a file descriptor.  */
 
@@ -47,6 +48,18 @@ public:
     return fd;
   }
 
+  /* Like release, but return a gdb_file_up that owns the file
+     descriptor.  On success, this scoped_fd will be released.  On
+     failure, return NULL and leave this scoped_fd in possession of
+     the fd.  */
+  gdb_file_up to_file (const char *mode) noexcept
+  {
+    gdb_file_up result (fdopen (m_fd, mode));
+    if (result != nullptr)
+      m_fd = -1;
+    return result;
+  }
+
   int get () const noexcept
   {
     return m_fd;
diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
index 7049e8968a..341e16bfc5 100644
--- a/gdb/dwarf-index-write.c
+++ b/gdb/dwarf-index-write.c
@@ -1565,23 +1565,21 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
 			   ? INDEX5_SUFFIX : INDEX4_SUFFIX));
   gdb::char_vector filename_temp = make_temp_filename (filename);
 
-  gdb::optional<scoped_fd> out_file_fd
-    (gdb::in_place, gdb_mkstemp_cloexec (filename_temp.data ()));
-  if (out_file_fd->get () == -1)
+  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
+     unlinked, because on MS-Windows one cannot delete a file that is
+     still open.  (Don't call anything here that might throw until
+     file_closer is created.)  So, we wrap the unlinker in an optional
+     and emplace it once we know the file name.  */
+  gdb::optional<gdb::unlinker> unlink_file;
+  scoped_fd out_file_fd (gdb_mkstemp_cloexec (filename_temp.data ()));
+  if (out_file_fd.get () == -1)
     perror_with_name (("mkstemp"));
 
-  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
+  gdb_file_up out_file = out_file_fd.to_file ("wb");
   if (out_file == nullptr)
     error (_("Can't open `%s' for writing"), filename_temp.data ());
 
-  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
-     unlinked, because on MS-Windows one cannot delete a file that is
-     still open.  (Don't call anything here that might throw until
-     file_closer is created.)  We don't need OUT_FILE_FD anymore, so we might
-     as well close it now.  */
-  out_file_fd.reset ();
-  gdb::unlinker unlink_file (filename_temp.data ());
-  gdb_file_up close_out_file (out_file);
+  unlink_file.emplace (filename_temp.data ());
 
   if (index_kind == dw_index_kind::DEBUG_NAMES)
     {
@@ -1589,44 +1587,45 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
 				+ basename + DEBUG_STR_SUFFIX);
       gdb::char_vector filename_str_temp = make_temp_filename (filename_str);
 
-      gdb::optional<scoped_fd> out_file_str_fd
-	(gdb::in_place, gdb_mkstemp_cloexec (filename_str_temp.data ()));
-      if (out_file_str_fd->get () == -1)
+      /* As above, arrange to unlink the file only after the file
+	 descriptor has been closed.  */
+      gdb::optional<gdb::unlinker> unlink_file_str;
+      scoped_fd out_file_str_fd
+	(gdb_mkstemp_cloexec (filename_str_temp.data ()));
+      if (out_file_str_fd.get () == -1)
         perror_with_name (("mkstemp"));
 
-      FILE *out_file_str
-	= gdb_fopen_cloexec (filename_str_temp.data (), "wb").release ();
+      gdb_file_up out_file_str = out_file_str_fd.to_file ("wb");
       if (out_file_str == nullptr)
 	error (_("Can't open `%s' for writing"), filename_str_temp.data ());
 
-      out_file_str_fd.reset ();
-      gdb::unlinker unlink_file_str (filename_str_temp.data ());
-      gdb_file_up close_out_file_str (out_file_str);
+      unlink_file_str.emplace (filename_str_temp.data ());
 
       const size_t total_len
-	= write_debug_names (dwarf2_per_objfile, out_file, out_file_str);
-      assert_file_size (out_file, filename_temp.data (), total_len);
+	= write_debug_names (dwarf2_per_objfile, out_file.get (),
+			     out_file_str.get ());
+      assert_file_size (out_file.get (), filename_temp.data (), total_len);
 
       /* We want to keep the file .debug_str file too.  */
-      unlink_file_str.keep ();
+      unlink_file_str->keep ();
 
       /* Close and move the str file in place.  */
-      close_out_file_str.reset ();
+      out_file_str.reset ();
       if (rename (filename_str_temp.data (), filename_str.c_str ()) != 0)
 	perror_with_name (("rename"));
     }
   else
     {
       const size_t total_len
-	= write_gdbindex (dwarf2_per_objfile, out_file);
-      assert_file_size (out_file, filename_temp.data (), total_len);
+	= write_gdbindex (dwarf2_per_objfile, out_file.get ());
+      assert_file_size (out_file.get (), filename_temp.data (), total_len);
     }
 
   /* We want to keep the file.  */
-  unlink_file.keep ();
+  unlink_file->keep ();
 
   /* Close and move the file in place.  */
-  close_out_file.reset ();
+  out_file.reset ();
   if (rename (filename_temp.data (), filename.c_str ()) != 0)
 	perror_with_name (("rename"));
 }
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index 4320b540cd..4690ae79f5 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -68,12 +68,29 @@ test_release ()
   SELF_CHECK (close (fd) == 0 || errno != EBADF);
 }
 
+/* Test that the file descriptor can be converted to a FILE *.  */
+static void
+test_to_file ()
+{
+  char filename[] = "scoped_fd-selftest-XXXXXX";
+
+  ::scoped_fd sfd (gdb_mkstemp_cloexec (filename));
+  SELF_CHECK (sfd.get () >= 0);
+
+  unlink (filename);
+  
+  gdb_file_up file = sfd.to_file ("rw");
+  SELF_CHECK (file != nullptr);
+  SELF_CHECK (sfd.get () == -1);
+}
+
 /* Run selftests.  */
 static void
 run_tests ()
 {
   test_destroy ();
   test_release ();
+  test_to_file ();
 }
 
 } /* namespace scoped_fd */
-- 
2.17.1

Re: [RFC 5/6] Do not reopen temporary files

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Wed, 26 Sep 2018 05:11:29 -0600
> 
> The current callers of mkostemp close the file descriptor and then
> re-open it with fopen.  It seemed better to me to continue to use the
> already-opened file descriptor, so this patch rearranges the code a
> little in order to do so.  It takes care to ensure that the files are
> only unlinked after the file descriptor in question is closed, as
> before.

Bother...

> -  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
> +  gdb_file_up out_file = out_file_fd.to_file ("wb");

There's a known bug in the MS-Windows (thus MinGW) implementation of
'fdopen': it ignores the 'b' and 't' flags.  So the preceding call to
'open' must itself specify the O_BINARY flag, or else this replacement
will introduce a subtle bug.

Thanks.

Re: [RFC 5/6] Do not reopen temporary files

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> -  FILE *out_file = gdb_fopen_cloexec (filename_temp.data (), "wb").release ();
>> +  gdb_file_up out_file = out_file_fd.to_file ("wb");

Eli> There's a known bug in the MS-Windows (thus MinGW) implementation of
Eli> 'fdopen': it ignores the 'b' and 't' flags.  So the preceding call to
Eli> 'open' must itself specify the O_BINARY flag, or else this replacement
Eli> will introduce a subtle bug.

Thanks.  I think I can pass O_BINARY to mkostemp.

Tom

Re: [RFC 5/6] Do not reopen temporary files

[Simon Marchi]

On 2018-09-26 7:11 a.m., Tom Tromey wrote:
> diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
> index 7049e8968a..341e16bfc5 100644
> --- a/gdb/dwarf-index-write.c
> +++ b/gdb/dwarf-index-write.c
> @@ -1565,23 +1565,21 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
>  			   ? INDEX5_SUFFIX : INDEX4_SUFFIX));
>    gdb::char_vector filename_temp = make_temp_filename (filename);
>  
> -  gdb::optional<scoped_fd> out_file_fd
> -    (gdb::in_place, gdb_mkstemp_cloexec (filename_temp.data ()));
> -  if (out_file_fd->get () == -1)
> +  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
> +     unlinked, because on MS-Windows one cannot delete a file that is
> +     still open.  (Don't call anything here that might throw until
> +     file_closer is created.)  So, we wrap the unlinker in an optional

I just noticed this refers to file_closer, a variable that doesn't exist anymore
(or has been renamed).

Otherwise this LGTM.

Simon

Re: [RFC 5/6] Do not reopen temporary files

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  /* Order matters here; we want FILE to be closed before FILENAME_TEMP is
>> +     unlinked, because on MS-Windows one cannot delete a file that is
>> +     still open.  (Don't call anything here that might throw until
>> +     file_closer is created.)  So, we wrap the unlinker in an optional

Simon> I just noticed this refers to file_closer, a variable that doesn't exist anymore
Simon> (or has been renamed).

Thanks.  I've removed the parenthesized sentence, because it no longer
applies.

Tom

[RFC 2/6] Move make_temp_filename to common/pathstuff.c

[Tom Tromey]

Currently make_temp_filename is a function local to
write_psymtabs_to_index.  This patch moves it to pathstuff.c so that
it can be used from other places in gdb.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* dwarf-index-write.c (write_psymtabs_to_index): Move
	make_temp_filename to common/pathstuff.c.
	* common/pathstuff.h (make_temp_filename): Declare.
	* common/pathstuff.c (make_temp_filename): New function, moved
	from dwarf-index-write.c.
---
 gdb/ChangeLog           |  8 ++++++++
 gdb/common/pathstuff.c  | 11 +++++++++++
 gdb/common/pathstuff.h  |  7 +++++++
 gdb/dwarf-index-write.c | 11 +----------
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index 6d8e53f4e1..48ff861eda 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -202,3 +202,14 @@ get_shell ()
 
   return ret;
 }
+
+/* See common/pathstuff.h.  */
+
+gdb::char_vector
+make_temp_filename (const std::string &f)
+{
+  gdb::char_vector filename_temp (f.length () + 8);
+  strcpy (filename_temp.data (), f.c_str ());
+  strcat (filename_temp.data () + f.size (), "-XXXXXX");
+  return filename_temp;
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
index 40fbda8d85..14fab04805 100644
--- a/gdb/common/pathstuff.h
+++ b/gdb/common/pathstuff.h
@@ -20,6 +20,8 @@
 #ifndef PATHSTUFF_H
 #define PATHSTUFF_H
 
+#include "common/byte-vector.h"
+
 /* Path utilities.  */
 
 /* Return the real path of FILENAME, expanding all the symbolic links.
@@ -68,4 +70,9 @@ extern std::string get_standard_cache_dir ();
 
 extern const char *get_shell ();
 
+/* Make a filename suitable to pass to mkstemp based on F (e.g.
+   /tmp/foo -> /tmp/foo-XXXXXX).  */
+
+extern gdb::char_vector make_temp_filename (const std::string &f);
+
 #endif /* PATHSTUFF_H */
diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
index 252032161f..dc8660e734 100644
--- a/gdb/dwarf-index-write.c
+++ b/gdb/dwarf-index-write.c
@@ -24,6 +24,7 @@
 #include "common/byte-vector.h"
 #include "common/filestuff.h"
 #include "common/gdb_unlinker.h"
+#include "common/pathstuff.h"
 #include "common/scoped_fd.h"
 #include "complaints.h"
 #include "dwarf-index-common.h"
@@ -1559,16 +1560,6 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
   if (stat (objfile_name (objfile), &st) < 0)
     perror_with_name (objfile_name (objfile));
 
-  /* Make a filename suitable to pass to mkstemp based on F (e.g.
-     /tmp/foo -> /tmp/foo-XXXXXX).  */
-  auto make_temp_filename = [] (const std::string &f) -> gdb::char_vector
-    {
-      gdb::char_vector filename_temp (f.length () + 8);
-      strcpy (filename_temp.data (), f.c_str ());
-      strcat (filename_temp.data () + f.size (), "-XXXXXX");
-      return filename_temp;
-    };
-
   std::string filename (std::string (dir) + SLASH_STRING + basename
 			+ (index_kind == dw_index_kind::DEBUG_NAMES
 			   ? INDEX5_SUFFIX : INDEX4_SUFFIX));
-- 
2.17.1

[RFC 4/6] Use mkostemp, not mkstemp

[Tom Tromey]

I noticed that gdb could leak file descriptors coming from mkstemp.
This patch fixes the problem by importing the gnulib mkostemp instead,
and then changing gdb to pass O_CLOEXEC.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* unittests/scoped_mmap-selftests.c (test_normal): Use
	gdb_mkstemp_cloexec.
	* unittests/scoped_fd-selftests.c (test_destroy, test_release):
	Use gdb_mkstemp_cloexec.
	* gnulib/aclocal-m4-deps.mk, gdb/gnulib/aclocal.m4,
	gdb/gnulib/config.in, gdb/gnulib/configure,
	gdb/gnulib/import/Makefile.am, gdb/gnulib/import/Makefile.in,
	gdb/gnulib/import/m4/gnulib-cache.m4,
	gdb/gnulib/import/m4/gnulib-comp.m4: Update.
	* gdb/gnulib/import/m4/mkostemp.m4: New file.
	* gdb/gnulib/import/m4/mkstemp.m4: Remove.
	* gdb/gnulib/import/mkostemp.c: New file.
	* gdb/gnulib/import/mkstemp.m4: Remove.
	* gdb/gnulib/update-gnulib.sh (IMPORTED_GNULIB_MODULES): Remove
	mkstemp, add mkostemp/
	* dwarf-index-write.c (write_psymtabs_to_index): Use
	gdb_mkstemp_cloexec.
	* common/filestuff.h (gdb_mkstemp_cloexec): New function.
---
 gdb/ChangeLog                               | 21 +++++
 gdb/common/filestuff.h                      | 11 +++
 gdb/dwarf-index-write.c                     |  4 +-
 gdb/gnulib/aclocal-m4-deps.mk               |  2 +-
 gdb/gnulib/aclocal.m4                       |  2 +-
 gdb/gnulib/config.in                        | 12 ++-
 gdb/gnulib/configure                        | 95 ++++-----------------
 gdb/gnulib/import/Makefile.am               | 10 +--
 gdb/gnulib/import/Makefile.in               | 18 ++--
 gdb/gnulib/import/m4/gnulib-cache.m4        |  4 +-
 gdb/gnulib/import/m4/gnulib-comp.m4         | 17 ++--
 gdb/gnulib/import/m4/mkostemp.m4            | 23 +++++
 gdb/gnulib/import/m4/mkstemp.m4             | 82 ------------------
 gdb/gnulib/import/{mkstemp.c => mkostemp.c} | 12 +--
 gdb/gnulib/update-gnulib.sh                 |  2 +-
 gdb/unittests/scoped_fd-selftests.c         |  5 +-
 gdb/unittests/scoped_mmap-selftests.c       |  3 +-
 17 files changed, 117 insertions(+), 206 deletions(-)
 create mode 100644 gdb/gnulib/import/m4/mkostemp.m4
 delete mode 100644 gdb/gnulib/import/m4/mkstemp.m4
 rename gdb/gnulib/import/{mkstemp.c => mkostemp.c} (79%)

diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index ecfc18d600..cc5852feec 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -20,6 +20,7 @@
 #define FILESTUFF_H
 
 #include <dirent.h>
+#include <fcntl.h>
 
 /* Note all the file descriptors which are open when this is called.
    These file descriptors will not be closed by close_most_fds.  */
@@ -48,6 +49,16 @@ extern void close_most_fds (void);
 extern int gdb_open_cloexec (const char *filename, int flags,
 			     /* mode_t */ unsigned long mode);
 
+/* Like mkstemp, but ensures that the file descriptor is
+   close-on-exec.  */
+
+static inline int
+gdb_mkstemp_cloexec (char *name_template)
+{
+  /* gnulib provides a mkostemp replacement if needed.  */
+  return mkostemp (name_template, O_CLOEXEC);
+}
+
 /* Convenience wrapper for the above, which takes the filename as an
    std::string.  */
 
diff --git a/gdb/dwarf-index-write.c b/gdb/dwarf-index-write.c
index dc8660e734..7049e8968a 100644
--- a/gdb/dwarf-index-write.c
+++ b/gdb/dwarf-index-write.c
@@ -1566,7 +1566,7 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
   gdb::char_vector filename_temp = make_temp_filename (filename);
 
   gdb::optional<scoped_fd> out_file_fd
-    (gdb::in_place, mkstemp (filename_temp.data ()));
+    (gdb::in_place, gdb_mkstemp_cloexec (filename_temp.data ()));
   if (out_file_fd->get () == -1)
     perror_with_name (("mkstemp"));
 
@@ -1590,7 +1590,7 @@ write_psymtabs_to_index (struct dwarf2_per_objfile *dwarf2_per_objfile,
       gdb::char_vector filename_str_temp = make_temp_filename (filename_str);
 
       gdb::optional<scoped_fd> out_file_str_fd
-	(gdb::in_place, mkstemp (filename_str_temp.data ()));
+	(gdb::in_place, gdb_mkstemp_cloexec (filename_str_temp.data ()));
       if (out_file_str_fd->get () == -1)
         perror_with_name (("mkstemp"));
 
diff --git a/gdb/gnulib/aclocal-m4-deps.mk b/gdb/gnulib/aclocal-m4-deps.mk
index d866b6de89..5b2c6cc5ea 100644
--- a/gdb/gnulib/aclocal-m4-deps.mk
+++ b/gdb/gnulib/aclocal-m4-deps.mk
@@ -80,7 +80,7 @@ aclocal_m4_deps = \
 	import/m4/mempcpy.m4 \
 	import/m4/memrchr.m4 \
 	import/m4/mkdir.m4 \
-	import/m4/mkstemp.m4 \
+	import/m4/mkostemp.m4 \
 	import/m4/mmap-anon.m4 \
 	import/m4/mode_t.m4 \
 	import/m4/msvc-inval.m4 \
diff --git a/gdb/gnulib/aclocal.m4 b/gdb/gnulib/aclocal.m4
index 740387ac34..861caf6692 100644
--- a/gdb/gnulib/aclocal.m4
+++ b/gdb/gnulib/aclocal.m4
@@ -1353,7 +1353,7 @@ m4_include([import/m4/memmem.m4])
 m4_include([import/m4/mempcpy.m4])
 m4_include([import/m4/memrchr.m4])
 m4_include([import/m4/mkdir.m4])
-m4_include([import/m4/mkstemp.m4])
+m4_include([import/m4/mkostemp.m4])
 m4_include([import/m4/mmap-anon.m4])
 m4_include([import/m4/mode_t.m4])
 m4_include([import/m4/msvc-inval.m4])
diff --git a/gdb/gnulib/config.in b/gdb/gnulib/config.in
index d32b192e0b..d23d208cb2 100644
--- a/gdb/gnulib/config.in
+++ b/gdb/gnulib/config.in
@@ -95,6 +95,10 @@
    whether the gnulib module getcwd shall be considered present. */
 #undef GNULIB_GETCWD
 
+/* Define to a C preprocessor expression that evaluates to 1 or 0, depending
+   whether the gnulib module mkostemp shall be considered present. */
+#undef GNULIB_MKOSTEMP
+
 /* Define to a C preprocessor expression that evaluates to 1 or 0, depending
    whether the gnulib module openat shall be considered present. */
 #undef GNULIB_OPENAT
@@ -199,8 +203,8 @@
 /* Define to 1 when the gnulib module memrchr should be tested. */
 #undef GNULIB_TEST_MEMRCHR
 
-/* Define to 1 when the gnulib module mkstemp should be tested. */
-#undef GNULIB_TEST_MKSTEMP
+/* Define to 1 when the gnulib module mkostemp should be tested. */
+#undef GNULIB_TEST_MKOSTEMP
 
 /* Define to 1 when the gnulib module open should be tested. */
 #undef GNULIB_TEST_OPEN
@@ -544,8 +548,8 @@
    when it succeeds. */
 #undef HAVE_MINIMALLY_WORKING_GETCWD
 
-/* Define to 1 if you have the 'mkstemp' function. */
-#undef HAVE_MKSTEMP
+/* Define to 1 if you have the 'mkostemp' function. */
+#undef HAVE_MKOSTEMP
 
 /* Define to 1 if you have the 'mprotect' function. */
 #undef HAVE_MPROTECT
diff --git a/gdb/gnulib/configure b/gdb/gnulib/configure
index dc6c5b6657..5d7f8aa004 100644
--- a/gdb/gnulib/configure
+++ b/gdb/gnulib/configure
@@ -3525,7 +3525,7 @@ gl_func_list="$gl_func_list mbsinit"
 gl_func_list="$gl_func_list mbrtowc"
 gl_header_list="$gl_header_list sys/mman.h"
 gl_func_list="$gl_func_list mprotect"
-gl_func_list="$gl_func_list mkstemp"
+gl_func_list="$gl_func_list mkostemp"
 gl_func_list="$gl_func_list openat"
 gl_func_list="$gl_func_list link"
 gl_func_list="$gl_func_list secure_getenv"
@@ -5841,7 +5841,7 @@ fi
   # Code from module mempcpy:
   # Code from module memrchr:
   # Code from module mkdir:
-  # Code from module mkstemp:
+  # Code from module mkostemp:
   # Code from module msvc-inval:
   # Code from module msvc-nothrow:
   # Code from module multiarch:
@@ -21893,116 +21893,51 @@ $as_echo "#define FUNC_MKDIR_DOT_BUG 1" >>confdefs.h
 
 
 
+
+
   :
 
 
 
 
 
-  if test $ac_cv_func_mkstemp = yes; then
-    { $as_echo "$as_me:${as_lineno-$LINENO}: checking for working mkstemp" >&5
-$as_echo_n "checking for working mkstemp... " >&6; }
-if ${gl_cv_func_working_mkstemp+:} false; then :
-  $as_echo_n "(cached) " >&6
-else
+  if test $ac_cv_func_mkostemp != yes; then
+    HAVE_MKOSTEMP=0
+  fi
 
-        mkdir conftest.mkstemp
-        if test "$cross_compiling" = yes; then :
-  case "$host_os" in
-                     # Guess yes on glibc systems.
-             *-gnu*) gl_cv_func_working_mkstemp="guessing yes" ;;
-                     # If we don't know, assume the worst.
-             *)      gl_cv_func_working_mkstemp="guessing no" ;;
-           esac
+  if test $HAVE_MKOSTEMP = 0; then
 
-else
-  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h.  */
-$ac_includes_default
-int
-main ()
-{
-int result = 0;
-              int i;
-              off_t large = (off_t) 4294967295u;
-              if (large < 0)
-                large = 2147483647;
-              umask (0);
-              for (i = 0; i < 70; i++)
-                {
-                  char templ[] = "conftest.mkstemp/coXXXXXX";
-                  int (*mkstemp_function) (char *) = mkstemp;
-                  int fd = mkstemp_function (templ);
-                  if (fd < 0)
-                    result |= 1;
-                  else
-                    {
-                      struct stat st;
-                      if (lseek (fd, large, SEEK_SET) != large)
-                        result |= 2;
-                      if (fstat (fd, &st) < 0)
-                        result |= 4;
-                      else if (st.st_mode & 0077)
-                        result |= 8;
-                      if (close (fd))
-                        result |= 16;
-                    }
-                }
-              return result;
-  ;
-  return 0;
-}
-_ACEOF
-if ac_fn_c_try_run "$LINENO"; then :
-  gl_cv_func_working_mkstemp=yes
-else
-  gl_cv_func_working_mkstemp=no
-fi
-rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
-  conftest.$ac_objext conftest.beam conftest.$ac_ext
-fi
 
-        rm -rf conftest.mkstemp
 
-fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gl_cv_func_working_mkstemp" >&5
-$as_echo "$gl_cv_func_working_mkstemp" >&6; }
-    case "$gl_cv_func_working_mkstemp" in
-      *yes) ;;
-      *)
-        REPLACE_MKSTEMP=1
-        ;;
-    esac
-  else
-    HAVE_MKSTEMP=0
-  fi
 
-  if test $HAVE_MKSTEMP = 0 || test $REPLACE_MKSTEMP = 1; then
 
 
 
 
+  gl_LIBOBJS="$gl_LIBOBJS mkostemp.$ac_objext"
 
 
 
+  fi
 
-  gl_LIBOBJS="$gl_LIBOBJS mkstemp.$ac_objext"
 
+cat >>confdefs.h <<_ACEOF
+#define GNULIB_MKOSTEMP 1
+_ACEOF
 
 
-  fi
 
 
 
 
 
-          GNULIB_MKSTEMP=1
+          GNULIB_MKOSTEMP=1
 
 
 
 
 
-$as_echo "#define GNULIB_TEST_MKSTEMP 1" >>confdefs.h
+$as_echo "#define GNULIB_TEST_MKOSTEMP 1" >>confdefs.h
 
 
 
diff --git a/gdb/gnulib/import/Makefile.am b/gdb/gnulib/import/Makefile.am
index d1bee0bc02..608c2c725c 100644
--- a/gdb/gnulib/import/Makefile.am
+++ b/gdb/gnulib/import/Makefile.am
@@ -21,7 +21,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkstemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
+# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkostemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
 
 AUTOMAKE_OPTIONS = 1.9.6 gnits
 
@@ -1223,14 +1223,14 @@ EXTRA_libgnu_a_SOURCES += mkdir.c
 
 ## end   gnulib module mkdir
 
-## begin gnulib module mkstemp
+## begin gnulib module mkostemp
 
 
-EXTRA_DIST += mkstemp.c
+EXTRA_DIST += mkostemp.c
 
-EXTRA_libgnu_a_SOURCES += mkstemp.c
+EXTRA_libgnu_a_SOURCES += mkostemp.c
 
-## end   gnulib module mkstemp
+## end   gnulib module mkostemp
 
 ## begin gnulib module msvc-inval
 
diff --git a/gdb/gnulib/import/Makefile.in b/gdb/gnulib/import/Makefile.in
index 7e4d278d91..8b0487ccc7 100644
--- a/gdb/gnulib/import/Makefile.in
+++ b/gdb/gnulib/import/Makefile.in
@@ -35,7 +35,7 @@
 # the same distribution terms as the rest of that program.
 #
 # Generated by gnulib-tool.
-# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkstemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
+# Reproduce by: gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkostemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
 
 
 
@@ -192,7 +192,7 @@ am__aclocal_m4_deps = $(top_srcdir)/import/m4/00gnulib.m4 \
 	$(top_srcdir)/import/m4/mempcpy.m4 \
 	$(top_srcdir)/import/m4/memrchr.m4 \
 	$(top_srcdir)/import/m4/mkdir.m4 \
-	$(top_srcdir)/import/m4/mkstemp.m4 \
+	$(top_srcdir)/import/m4/mkostemp.m4 \
 	$(top_srcdir)/import/m4/mmap-anon.m4 \
 	$(top_srcdir)/import/m4/mode_t.m4 \
 	$(top_srcdir)/import/m4/msvc-inval.m4 \
@@ -1515,7 +1515,7 @@ EXTRA_DIST = m4/gnulib-cache.m4 alloca.c alloca.in.h arpa_inet.in.h \
 	malloca.valgrind math.in.h mbrtowc.c mbsinit.c \
 	mbsrtowcs-impl.h mbsrtowcs-state.c mbsrtowcs.c memchr.c \
 	memchr.valgrind memmem.c str-two-way.h mempcpy.c memrchr.c \
-	mkdir.c mkstemp.c msvc-inval.c msvc-inval.h msvc-nothrow.c \
+	mkdir.c mkostemp.c msvc-inval.c msvc-inval.h msvc-nothrow.c \
 	msvc-nothrow.h netinet_in.in.h open.c openat.c openat.h \
 	dirent-private.h opendir.c pathmax.h rawmemchr.c \
 	rawmemchr.valgrind dirent-private.h readdir.c readlink.c \
@@ -1587,11 +1587,11 @@ EXTRA_libgnu_a_SOURCES = alloca.c openat-proc.c canonicalize-lgpl.c \
 	gettimeofday.c glob.c inet_ntop.c isnan.c isnand.c isnan.c \
 	isnanl.c lstat.c malloc.c mbrtowc.c mbsinit.c \
 	mbsrtowcs-state.c mbsrtowcs.c memchr.c memmem.c mempcpy.c \
-	memrchr.c mkdir.c mkstemp.c msvc-inval.c msvc-nothrow.c open.c \
-	openat.c opendir.c rawmemchr.c readdir.c readlink.c realloc.c \
-	rename.c rewinddir.c rmdir.c secure_getenv.c setenv.c stat.c \
-	strchrnul.c strdup.c strerror.c strerror-override.c strstr.c \
-	strtok_r.c unsetenv.c
+	memrchr.c mkdir.c mkostemp.c msvc-inval.c msvc-nothrow.c \
+	open.c openat.c opendir.c rawmemchr.c readdir.c readlink.c \
+	realloc.c rename.c rewinddir.c rmdir.c secure_getenv.c \
+	setenv.c stat.c strchrnul.c strdup.c strerror.c \
+	strerror-override.c strstr.c strtok_r.c unsetenv.c
 
 # Use this preprocessor expression to decide whether #include_next works.
 # Do not rely on a 'configure'-time test for this, since the expression
@@ -1722,7 +1722,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/mempcpy.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/memrchr.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/mkdir.Po@am__quote@
-@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/mkstemp.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/mkostemp.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/msvc-inval.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/msvc-nothrow.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/open.Po@am__quote@
diff --git a/gdb/gnulib/import/m4/gnulib-cache.m4 b/gdb/gnulib/import/m4/gnulib-cache.m4
index 442aad5563..8cefb67be7 100644
--- a/gdb/gnulib/import/m4/gnulib-cache.m4
+++ b/gdb/gnulib/import/m4/gnulib-cache.m4
@@ -27,7 +27,7 @@
 
 
 # Specification in the form of a command-line invocation:
-#   gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkstemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
+#   gnulib-tool --import --lib=libgnu --source-base=import --m4-base=import/m4 --doc-base=doc --tests-base=tests --aux-dir=import/extra --no-conditional-dependencies --no-libtool --macro-prefix=gl --no-vc-files alloca canonicalize-lgpl dirent dirfd errno fnmatch-gnu frexpl getcwd glob inet_ntop inttypes limits-h lstat memchr memmem mkdir mkostemp pathmax rawmemchr readlink rename setenv signal-h strchrnul strstr strtok_r sys_stat unistd unsetenv update-copyright wchar wctype-h
 
 # Specification in the form of a few gnulib-tool.m4 macro invocations:
 gl_LOCAL_DIR([])
@@ -48,7 +48,7 @@ gl_MODULES([
   memchr
   memmem
   mkdir
-  mkstemp
+  mkostemp
   pathmax
   rawmemchr
   readlink
diff --git a/gdb/gnulib/import/m4/gnulib-comp.m4 b/gdb/gnulib/import/m4/gnulib-comp.m4
index 21e4383c34..2c55958eb7 100644
--- a/gdb/gnulib/import/m4/gnulib-comp.m4
+++ b/gdb/gnulib/import/m4/gnulib-comp.m4
@@ -121,7 +121,7 @@ AC_DEFUN([gl_EARLY],
   # Code from module mempcpy:
   # Code from module memrchr:
   # Code from module mkdir:
-  # Code from module mkstemp:
+  # Code from module mkostemp:
   # Code from module msvc-inval:
   # Code from module msvc-nothrow:
   # Code from module multiarch:
@@ -444,12 +444,13 @@ AC_DEFUN([gl_INIT],
   if test $REPLACE_MKDIR = 1; then
     AC_LIBOBJ([mkdir])
   fi
-  gl_FUNC_MKSTEMP
-  if test $HAVE_MKSTEMP = 0 || test $REPLACE_MKSTEMP = 1; then
-    AC_LIBOBJ([mkstemp])
-    gl_PREREQ_MKSTEMP
+  gl_FUNC_MKOSTEMP
+  if test $HAVE_MKOSTEMP = 0; then
+    AC_LIBOBJ([mkostemp])
+    gl_PREREQ_MKOSTEMP
   fi
-  gl_STDLIB_MODULE_INDICATOR([mkstemp])
+  gl_MODULE_INDICATOR([mkostemp])
+  gl_STDLIB_MODULE_INDICATOR([mkostemp])
   AC_REQUIRE([gl_MSVC_INVAL])
   if test $HAVE_MSVC_INVALID_PARAMETER_HANDLER = 1; then
     AC_LIBOBJ([msvc-inval])
@@ -844,7 +845,7 @@ AC_DEFUN([gl_FILE_LIST], [
   lib/mempcpy.c
   lib/memrchr.c
   lib/mkdir.c
-  lib/mkstemp.c
+  lib/mkostemp.c
   lib/msvc-inval.c
   lib/msvc-inval.h
   lib/msvc-nothrow.c
@@ -991,7 +992,7 @@ AC_DEFUN([gl_FILE_LIST], [
   m4/mempcpy.m4
   m4/memrchr.m4
   m4/mkdir.m4
-  m4/mkstemp.m4
+  m4/mkostemp.m4
   m4/mmap-anon.m4
   m4/mode_t.m4
   m4/msvc-inval.m4
diff --git a/gdb/gnulib/import/m4/mkostemp.m4 b/gdb/gnulib/import/m4/mkostemp.m4
new file mode 100644
index 0000000000..1f44a0390a
--- /dev/null
+++ b/gdb/gnulib/import/m4/mkostemp.m4
@@ -0,0 +1,23 @@
+# mkostemp.m4 serial 2
+dnl Copyright (C) 2009-2016 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_FUNC_MKOSTEMP],
+[
+  AC_REQUIRE([gl_STDLIB_H_DEFAULTS])
+
+  dnl Persuade glibc <stdlib.h> to declare mkostemp().
+  AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
+
+  AC_CHECK_FUNCS_ONCE([mkostemp])
+  if test $ac_cv_func_mkostemp != yes; then
+    HAVE_MKOSTEMP=0
+  fi
+])
+
+# Prerequisites of lib/mkostemp.c.
+AC_DEFUN([gl_PREREQ_MKOSTEMP],
+[
+])
diff --git a/gdb/gnulib/import/m4/mkstemp.m4 b/gdb/gnulib/import/m4/mkstemp.m4
deleted file mode 100644
index 131e4a7b26..0000000000
--- a/gdb/gnulib/import/m4/mkstemp.m4
+++ /dev/null
@@ -1,82 +0,0 @@
-#serial 23
-
-# Copyright (C) 2001, 2003-2007, 2009-2016 Free Software Foundation, Inc.
-# This file is free software; the Free Software Foundation
-# gives unlimited permission to copy and/or distribute it,
-# with or without modifications, as long as this notice is preserved.
-
-# On some hosts (e.g., HP-UX 10.20, SunOS 4.1.4, Solaris 2.5.1), mkstemp has a
-# silly limit that it can create no more than 26 files from a given template.
-# Other systems lack mkstemp altogether.
-# On OSF1/Tru64 V4.0F, the system-provided mkstemp function can create
-# only 32 files per process.
-# On some hosts, mkstemp creates files with mode 0666, which is a security
-# problem and a violation of POSIX 2008.
-# On systems like the above, arrange to use the replacement function.
-AC_DEFUN([gl_FUNC_MKSTEMP],
-[
-  AC_REQUIRE([gl_STDLIB_H_DEFAULTS])
-  AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
-
-  AC_CHECK_FUNCS_ONCE([mkstemp])
-  if test $ac_cv_func_mkstemp = yes; then
-    AC_CACHE_CHECK([for working mkstemp],
-      [gl_cv_func_working_mkstemp],
-      [
-        mkdir conftest.mkstemp
-        AC_RUN_IFELSE(
-          [AC_LANG_PROGRAM(
-            [AC_INCLUDES_DEFAULT],
-            [[int result = 0;
-              int i;
-              off_t large = (off_t) 4294967295u;
-              if (large < 0)
-                large = 2147483647;
-              umask (0);
-              for (i = 0; i < 70; i++)
-                {
-                  char templ[] = "conftest.mkstemp/coXXXXXX";
-                  int (*mkstemp_function) (char *) = mkstemp;
-                  int fd = mkstemp_function (templ);
-                  if (fd < 0)
-                    result |= 1;
-                  else
-                    {
-                      struct stat st;
-                      if (lseek (fd, large, SEEK_SET) != large)
-                        result |= 2;
-                      if (fstat (fd, &st) < 0)
-                        result |= 4;
-                      else if (st.st_mode & 0077)
-                        result |= 8;
-                      if (close (fd))
-                        result |= 16;
-                    }
-                }
-              return result;]])],
-          [gl_cv_func_working_mkstemp=yes],
-          [gl_cv_func_working_mkstemp=no],
-          [case "$host_os" in
-                     # Guess yes on glibc systems.
-             *-gnu*) gl_cv_func_working_mkstemp="guessing yes" ;;
-                     # If we don't know, assume the worst.
-             *)      gl_cv_func_working_mkstemp="guessing no" ;;
-           esac
-          ])
-        rm -rf conftest.mkstemp
-      ])
-    case "$gl_cv_func_working_mkstemp" in
-      *yes) ;;
-      *)
-        REPLACE_MKSTEMP=1
-        ;;
-    esac
-  else
-    HAVE_MKSTEMP=0
-  fi
-])
-
-# Prerequisites of lib/mkstemp.c.
-AC_DEFUN([gl_PREREQ_MKSTEMP],
-[
-])
diff --git a/gdb/gnulib/import/mkstemp.c b/gdb/gnulib/import/mkostemp.c
similarity index 79%
rename from gdb/gnulib/import/mkstemp.c
rename to gdb/gnulib/import/mkostemp.c
index 90ed78e49e..31c3e4837f 100644
--- a/gdb/gnulib/import/mkstemp.c
+++ b/gdb/gnulib/import/mkostemp.c
@@ -24,7 +24,7 @@
 #if !_LIBC
 # include "tempname.h"
 # define __gen_tempname gen_tempname
-# ifndef __GT_FILE
+# ifndef __GTFILE
 #  define __GT_FILE GT_FILE
 # endif
 #endif
@@ -38,13 +38,9 @@
 /* Generate a unique temporary file name from XTEMPLATE.
    The last six characters of XTEMPLATE must be "XXXXXX";
    they are replaced with a string that makes the file name unique.
-   Then open the file and return a fd.
-
-   If you are creating temporary files which will later be removed,
-   consider using the clean-temp module, which avoids several pitfalls
-   of using mkstemp directly. */
+   Then open the file and return a fd. */
 int
-mkstemp (char *xtemplate)
+mkostemp (char *xtemplate, int flags)
 {
-  return __gen_tempname (xtemplate, 0, 0, __GT_FILE);
+  return __gen_tempname (xtemplate, 0, flags, __GT_FILE);
 }
diff --git a/gdb/gnulib/update-gnulib.sh b/gdb/gnulib/update-gnulib.sh
index 09933ab9fe..1e9c37b112 100755
--- a/gdb/gnulib/update-gnulib.sh
+++ b/gdb/gnulib/update-gnulib.sh
@@ -46,7 +46,7 @@ IMPORTED_GNULIB_MODULES="\
     memchr \
     memmem \
     mkdir \
-    mkstemp \
+    mkostemp \
     pathmax \
     rawmemchr \
     readlink \
diff --git a/gdb/unittests/scoped_fd-selftests.c b/gdb/unittests/scoped_fd-selftests.c
index 4d7454134a..4320b540cd 100644
--- a/gdb/unittests/scoped_fd-selftests.c
+++ b/gdb/unittests/scoped_fd-selftests.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 
+#include "common/filestuff.h"
 #include "common/scoped_fd.h"
 #include "config.h"
 
@@ -34,7 +35,7 @@ static void
 test_destroy ()
 {
   char filename[] = "scoped_fd-selftest-XXXXXX";
-  int fd = mkstemp (filename);
+  int fd = gdb_mkstemp_cloexec (filename);
   SELF_CHECK (fd >= 0);
 
   unlink (filename);
@@ -53,7 +54,7 @@ static void
 test_release ()
 {
   char filename[] = "scoped_fd-selftest-XXXXXX";
-  int fd = mkstemp (filename);
+  int fd = gdb_mkstemp_cloexec (filename);
   SELF_CHECK (fd >= 0);
 
   unlink (filename);
diff --git a/gdb/unittests/scoped_mmap-selftests.c b/gdb/unittests/scoped_mmap-selftests.c
index e9d4afdffc..9c1707e4fd 100644
--- a/gdb/unittests/scoped_mmap-selftests.c
+++ b/gdb/unittests/scoped_mmap-selftests.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 
+#include "common/filestuff.h"
 #include "common/scoped_mmap.h"
 #include "config.h"
 
@@ -88,7 +89,7 @@ static void
 test_normal ()
 {
   char filename[] = "scoped_mmapped_file-selftest-XXXXXX";
-  int fd = mkstemp (filename);
+  int fd = gdb_mkstemp_cloexec (filename);
   SELF_CHECK (fd >= 0);
 
   SELF_CHECK (write (fd, "Hello!", 7) == 7);
-- 
2.17.1

Re: [RFC 4/6] Use mkostemp, not mkstemp

[Simon Marchi]

On 2018-09-26 7:11 a.m., Tom Tromey wrote:
> I noticed that gdb could leak file descriptors coming from mkstemp.
> This patch fixes the problem by importing the gnulib mkostemp instead,
> and then changing gdb to pass O_CLOEXEC.

While this looks like the correct thing to do, I am curious to know if
you encountered an actual issue or this is theoretical.  I don't see
how a fork/exec could happen while one of these temp files are open.

Simon

Re: [RFC 4/6] Use mkostemp, not mkstemp

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 2018-09-26 7:11 a.m., Tom Tromey wrote:
>> I noticed that gdb could leak file descriptors coming from mkstemp.
>> This patch fixes the problem by importing the gnulib mkostemp instead,
>> and then changing gdb to pass O_CLOEXEC.

Simon> While this looks like the correct thing to do, I am curious to know if
Simon> you encountered an actual issue or this is theoretical.  I don't see
Simon> how a fork/exec could happen while one of these temp files are open.

It can happen if there is a Python or Guile thread that fork+execs.
It seems like it would be hard to observe, but on the other hand, fixing
it seems pretty easy.

Tom

Re: [RFC 4/6] Use mkostemp, not mkstemp

[Simon Marchi]

On 2018-09-29 8:49 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> On 2018-09-26 7:11 a.m., Tom Tromey wrote:
>>> I noticed that gdb could leak file descriptors coming from mkstemp.
>>> This patch fixes the problem by importing the gnulib mkostemp instead,
>>> and then changing gdb to pass O_CLOEXEC.
> 
> Simon> While this looks like the correct thing to do, I am curious to know if
> Simon> you encountered an actual issue or this is theoretical.  I don't see
> Simon> how a fork/exec could happen while one of these temp files are open.
> 
> It can happen if there is a Python or Guile thread that fork+execs.
> It seems like it would be hard to observe, but on the other hand, fixing
> it seems pretty easy.

Ah I see, I was only thinking about the exec when you use "run".

Thanks!

Simon

[RFC 3/6] Move mkdir_recursive to common/filestuff.c

[Tom Tromey]

This moves mkdir_recursive from dwarf-index-cache.c to
common/filestuff.c, and also changes it to return a boolean that says
whether or not it worked.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	* unittests/mkdir-recursive-selftests.c: New file.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/mkdir-recursive-selftests.c.
	* dwarf-index-cache.c (mkdir_recursive): Move to
	common/filestuff.c.
	(index_cache::store): Check return value of mkdir_recursive.
	(create_dir_and_check, test_mkdir_recursive): Move to new file.
	(_initialize_index_cache): Don't register test.
	* common/filestuff.h (mkdir_recursive): Declare.
	* common/filestuff.c (mkdir_recursive): Move from
	dwarf-index-cache.c.  Return bool.
---
 gdb/ChangeLog           |  14 +++++
 gdb/Makefile.in         |   1 +
 gdb/common/filestuff.c  |  45 ++++++++++++++++
 gdb/common/filestuff.h  |  10 ++++
 gdb/dwarf-index-cache.c | 114 +++-------------------------------------
 5 files changed, 76 insertions(+), 108 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3b158fa1db..69c4c49a97 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -419,6 +419,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/parse-connection-spec-selftests.c \
 	unittests/ptid-selftests.c \
+	unittests/mkdir-recursive-selftests.c \
 	unittests/rsp-low-selftests.c \
 	unittests/scoped_fd-selftests.c \
 	unittests/scoped_mmap-selftests.c \
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index fa10165a7c..763def7643 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -448,3 +448,48 @@ is_regular_file (const char *name, int *errno_ptr)
     *errno_ptr = EINVAL;
   return false;
 }
+
+/* See common/filestuff.h.  */
+
+bool
+mkdir_recursive (const char *dir)
+{
+  gdb::unique_xmalloc_ptr<char> holder (xstrdup (dir));
+  char * const start = holder.get ();
+  char *component_start = start;
+  char *component_end = start;
+
+  while (1)
+    {
+      /* Find the beginning of the next component.  */
+      while (*component_start == '/')
+	component_start++;
+
+      /* Are we done?  */
+      if (*component_start == '\0')
+	return true;
+
+      /* Find the slash or null-terminator after this component.  */
+      component_end = component_start;
+      while (*component_end != '/' && *component_end != '\0')
+	component_end++;
+
+      /* Temporarily replace the slash with a null terminator, so we can create
+         the directory up to this component.  */
+      char saved_char = *component_end;
+      *component_end = '\0';
+
+      /* If we get EEXIST and the existing path is a directory, then we're
+         happy.  If it exists, but it's a regular file and this is not the last
+         component, we'll fail at the next component.  If this is the last
+         component, the caller will fail with ENOTDIR when trying to
+         open/create a file under that path.  */
+      if (mkdir (start, 0700) != 0)
+	if (errno != EEXIST)
+	  return false;
+
+      /* Restore the overwritten char.  */
+      *component_end = saved_char;
+      component_start = component_end;
+    }
+}
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index e9328f5358..ecfc18d600 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -122,4 +122,14 @@ typedef std::unique_ptr<DIR, gdb_dir_deleter> gdb_dir_up;
    we're expecting a regular file.  */
 extern bool is_regular_file (const char *name, int *errno_ptr);
 
+
+/* A cheap (as in low-quality) recursive mkdir.  Try to create all the
+   parents directories up to DIR and DIR itself.  Stop if we hit an
+   error along the way.  There is no attempt to remove created
+   directories in case of failure.
+
+   Returns false on failure and sets errno.  */
+
+extern bool mkdir_recursive (const char *dir);
+
 #endif /* FILESTUFF_H */
diff --git a/gdb/dwarf-index-cache.c b/gdb/dwarf-index-cache.c
index 966630b60c..bac98f9db8 100644
--- a/gdb/dwarf-index-cache.c
+++ b/gdb/dwarf-index-cache.c
@@ -45,53 +45,6 @@ index_cache global_index_cache;
 static cmd_list_element *set_index_cache_prefix_list;
 static cmd_list_element *show_index_cache_prefix_list;
 
-/* A cheap (as in low-quality) recursive mkdir.  Try to create all the parents
-   directories up to DIR and DIR itself.  Stop if we hit an error along the way.
-   There is no attempt to remove created directories in case of failure.  */
-
-static void
-mkdir_recursive (const char *dir)
-{
-  gdb::unique_xmalloc_ptr<char> holder (xstrdup (dir));
-  char * const start = holder.get ();
-  char *component_start = start;
-  char *component_end = start;
-
-  while (1)
-    {
-      /* Find the beginning of the next component.  */
-      while (*component_start == '/')
-	component_start++;
-
-      /* Are we done?  */
-      if (*component_start == '\0')
-	return;
-
-      /* Find the slash or null-terminator after this component.  */
-      component_end = component_start;
-      while (*component_end != '/' && *component_end != '\0')
-	component_end++;
-
-      /* Temporarily replace the slash with a null terminator, so we can create
-         the directory up to this component.  */
-      char saved_char = *component_end;
-      *component_end = '\0';
-
-      /* If we get EEXIST and the existing path is a directory, then we're
-         happy.  If it exists, but it's a regular file and this is not the last
-         component, we'll fail at the next component.  If this is the last
-         component, the caller will fail with ENOTDIR when trying to
-         open/create a file under that path.  */
-      if (mkdir (start, 0700) != 0)
-	if (errno != EEXIST)
-	  return;
-
-      /* Restore the overwritten char.  */
-      *component_end = saved_char;
-      component_start = component_end;
-    }
-}
-
 /* Default destructor of index_cache_resource.  */
 index_cache_resource::~index_cache_resource () = default;
 
@@ -160,7 +113,12 @@ index_cache::store (struct dwarf2_per_objfile *dwarf2_per_objfile)
   TRY
     {
       /* Try to create the containing directory.  */
-      mkdir_recursive (m_dir.c_str ());
+      if (!mkdir_recursive (m_dir.c_str ()))
+	{
+	  warning (_("index cache: could not make cache directory: %s\n"),
+		   safe_strerror (errno));
+	  return;
+	}
 
       if (debug_index_cache)
         printf_unfiltered ("index cache: writing index cache for objfile %s\n",
@@ -346,62 +304,6 @@ show_index_cache_stats_command (const char *arg, int from_tty)
 		     indent, global_index_cache.n_misses ());
 }
 
-#if GDB_SELF_TEST && defined (HAVE_MKDTEMP)
-namespace selftests
-{
-
-/* Try to create DIR using mkdir_recursive and make sure it exists.  */
-
-static bool
-create_dir_and_check (const char *dir)
-{
-  mkdir_recursive (dir);
-
-  struct stat st;
-  if (stat (dir, &st) != 0)
-    perror_with_name (("stat"));
-
-  return (st.st_mode & S_IFDIR) != 0;
-}
-
-/* Test mkdir_recursive.  */
-
-static void
-test_mkdir_recursive ()
-{
-  char base[] = "/tmp/gdb-selftests-XXXXXX";
-
-  if (mkdtemp (base) == NULL)
-    perror_with_name (("mkdtemp"));
-
-  /* Try not to leave leftover directories.  */
-  struct cleanup_dirs {
-    cleanup_dirs (const char *base)
-      : m_base (base)
-    {}
-
-    ~cleanup_dirs () {
-      rmdir (string_printf ("%s/a/b/c/d/e", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b/c/d", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b/c", m_base).c_str ());
-      rmdir (string_printf ("%s/a/b", m_base).c_str ());
-      rmdir (string_printf ("%s/a", m_base).c_str ());
-      rmdir (m_base);
-    }
-
-  private:
-    const char *m_base;
-  } cleanup_dirs (base);
-
-  std::string dir = string_printf ("%s/a/b", base);
-  SELF_CHECK (create_dir_and_check (dir.c_str ()));
-
-  dir = string_printf ("%s/a/b/c//d/e/", base);
-  SELF_CHECK (create_dir_and_check (dir.c_str ()));
-}
-}
-#endif /*  GDB_SELF_TEST && defined (HAVE_MKDTEMP) */
-
 void
 _initialize_index_cache ()
 {
@@ -456,8 +358,4 @@ _initialize_index_cache ()
 When non-zero, debugging output for the index cache is displayed."),
 			    NULL, NULL,
 			    &setdebuglist, &showdebuglist);
-
-#if GDB_SELF_TEST && defined (HAVE_MKDTEMP)
-  selftests::register_test ("mkdir_recursive", selftests::test_mkdir_recursive);
-#endif
 }
-- 
2.17.1

Re: [RFC 3/6] Move mkdir_recursive to common/filestuff.c

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> 	* unittests/mkdir-recursive-selftests.c: New file.

I forgot to git add this file.
I'll send it soon.

Tom

Re: [RFC 3/6] Move mkdir_recursive to common/filestuff.c

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I forgot to git add this file.
Tom> I'll send it soon.

This is just the missing part of this patch.

Tom

commit 7c8093b561466131da4fd9ad30d93b3640615426
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Sep 26 16:16:55 2018 -0600

    add the test

diff --git a/gdb/unittests/mkdir-recursive-selftests.c b/gdb/unittests/mkdir-recursive-selftests.c
new file mode 100644
index 0000000000..d501f8e081
--- /dev/null
+++ b/gdb/unittests/mkdir-recursive-selftests.c
@@ -0,0 +1,89 @@
+/* Self tests for scoped_fd for GDB, the GNU debugger.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+
+#include "common/filestuff.h"
+#include "selftest.h"
+
+namespace selftests {
+namespace mkdir_recursive {
+
+/* Try to create DIR using mkdir_recursive and make sure it exists.  */
+
+static bool
+create_dir_and_check (const char *dir)
+{
+  ::mkdir_recursive (dir);
+
+  struct stat st;
+  if (stat (dir, &st) != 0)
+    perror_with_name (("stat"));
+
+  return (st.st_mode & S_IFDIR) != 0;
+}
+
+/* Test mkdir_recursive.  */
+
+static void
+test ()
+{
+  char base[] = "/tmp/gdb-selftests-XXXXXX";
+
+  if (mkdtemp (base) == NULL)
+    perror_with_name (("mkdtemp"));
+
+  /* Try not to leave leftover directories.  */
+  struct cleanup_dirs {
+    cleanup_dirs (const char *base)
+      : m_base (base)
+    {}
+
+    ~cleanup_dirs () {
+      rmdir (string_printf ("%s/a/b/c/d/e", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b/c/d", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b/c", m_base).c_str ());
+      rmdir (string_printf ("%s/a/b", m_base).c_str ());
+      rmdir (string_printf ("%s/a", m_base).c_str ());
+      rmdir (m_base);
+    }
+
+  private:
+    const char *m_base;
+  } cleanup_dirs (base);
+
+  std::string dir = string_printf ("%s/a/b", base);
+  SELF_CHECK (create_dir_and_check (dir.c_str ()));
+
+  dir = string_printf ("%s/a/b/c//d/e/", base);
+  SELF_CHECK (create_dir_and_check (dir.c_str ()));
+}
+
+}
+}
+
+void
+_initialize_mkdir_recursive_selftests ()
+{
+#if defined (HAVE_MKDTEMP)
+  selftests::register_test ("mkdir_recursive",
+			    selftests::mkdir_recursive::test);
+#endif
+}
+

[RFC 6/6] Cache a copy of the user's shell on macOS

[Tom Tromey]

Recent versions of macOS have a feature called System Integrity
Protection.  Among other things, This feature prevents ptrace from
tracing certain programs --- for example, the programs in /bin, which
includes typical shells.

This means that startup-with-shell does not work properly.  This is PR
cli/23364.  Currently there is a workaround in gdb to disable
startup-with-shell when this feature might be in use.

This patch changes gdb to be a bit more precise about when
startup-with-shell will not work, by checking whether the shell
executable is restricted.

If the shell is restricted, then this patch will also cause gdb to
cache a copy of the shell in the gdb cache directory, and then reset
the SHELL environment variable to point to this copy.  This lets
startup-with-shell work again.

Tested on High Sierra by trying to start a program using redirection,
and by running startup-with-shell.exp.

gdb/ChangeLog
2018-09-26  Tom Tromey  <tom@tromey.com>

	PR cli/23364:
	* darwin-nat.c (copied_shell): New global.
	(may_have_sip): Rename from should_disable_startup_with_shell.
	(copy_shell_to_cache, maybe_cache_shell): New functions.
	(darwin_nat_target::create_inferior): Update.  Use
	copied_shell.
---
 gdb/ChangeLog    |   9 +++
 gdb/darwin-nat.c | 152 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 154 insertions(+), 7 deletions(-)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index eee9380d65..0e62ff4eae 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -38,6 +38,7 @@
 #include "bfd.h"
 #include "bfd/mach-o.h"
 
+#include <copyfile.h>
 #include <sys/ptrace.h>
 #include <sys/signal.h>
 #include <setjmp.h>
@@ -61,7 +62,11 @@
 #include <mach/port.h>
 
 #include "darwin-nat.h"
+#include "filenames.h"
 #include "common/filestuff.h"
+#include "common/gdb_unlinker.h"
+#include "common/pathstuff.h"
+#include "common/scoped_fd.h"
 #include "nat/fork-inferior.h"
 
 /* Quick overview.
@@ -119,6 +124,10 @@ static int enable_mach_exceptions;
 /* Inferior that should report a fake stop event.  */
 static struct inferior *darwin_inf_fake_stop;
 
+/* If non-NULL, the shell we actually invoke.  See maybe_cache_shell
+   for details.  */
+static const char *copied_shell;
+
 #define PAGE_TRUNC(x) ((x) & ~(mach_page_size - 1))
 #define PAGE_ROUND(x) PAGE_TRUNC((x) + mach_page_size - 1)
 
@@ -1854,10 +1863,11 @@ darwin_execvp (const char *file, char * const argv[], char * const env[])
   posix_spawnp (NULL, argv[0], NULL, &attr, argv, env);
 }
 
-/* Read kernel version, and return TRUE on Sierra or later.  */
+/* Read kernel version, and return TRUE if this host may have System
+   Integrity Protection (Sierra or later).  */
 
 static bool
-should_disable_startup_with_shell ()
+may_have_sip ()
 {
   char str[16];
   size_t sz = sizeof (str);
@@ -1873,6 +1883,132 @@ should_disable_startup_with_shell ()
   return false;
 }
 
+/* A helper for maybe_cache_shell.  This copies the shell to the
+   cache.  It will throw an exception on any failure.  */
+
+static void
+copy_shell_to_cache (const char *shell, const std::string &new_name)
+{
+  scoped_fd from_fd (gdb_open_cloexec (shell, O_RDONLY, 0));
+  if (from_fd.get () < 0)
+    error (_("Could not open shell (%s) for reading: %s"),
+	   shell, safe_strerror (errno));
+
+  std::string new_dir = ldirname (new_name.c_str ());
+  if (!mkdir_recursive (new_dir.c_str ()))
+    error (_("Could not make cache directory \"%s\": %s"),
+	   new_dir.c_str (), safe_strerror (errno));
+
+  gdb::char_vector temp_name = make_temp_filename (new_name);
+  scoped_fd to_fd (gdb_mkstemp_cloexec (&temp_name[0]));
+  gdb::unlinker unlink_file_on_error (temp_name.data ());
+
+  if (to_fd.get () < 0)
+    error (_("Could not open temporary file \"%s\" for writing: %s"),
+	   temp_name.data (), safe_strerror (errno));
+
+  if (fcopyfile (from_fd.get (), to_fd.get (), nullptr,
+		 COPYFILE_STAT | COPYFILE_DATA) != 0)
+    error (_("Could not copy shell to cache as \"%s\": %s"),
+	   temp_name.data (), safe_strerror (errno));
+
+  /* Be sure that the caching is atomic so that we don't get bad
+     results from multiple copies of gdb running at the same time.  */
+  if (rename (temp_name.data (), new_name.c_str ()) != 0)
+    error (_("Could not rename shell cache file to \"%s\": %s"),
+	   new_name.c_str (), safe_strerror (errno));
+
+  unlink_file_on_error.keep ();
+}
+
+/* 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))
+    new_name.push_back ('/');
+  new_name.append (shell);
+
+  /* Maybe it was cached by some earlier gdb.  */
+  if (stat (new_name.c_str (), &sb) != 0 || !S_ISREG (sb.st_mode))
+    {
+      TRY
+	{
+	  copy_shell_to_cache (shell, new_name);
+	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  warning (_("This version of macOS has System Integrity Protection.\n\
+Because `startup-with-shell' is enabled, gdb tried to work around SIP by\n\
+caching a copy of your shell.  However, this failed:\n\
+%s\n\
+If you correct the problem, gdb will automatically try again the next time\n\
+you \"run\".  To prevent these attempts, you can use:\n\
+    set startup-with-shell off"),
+		   ex.message);
+	  return false;
+	}
+      END_CATCH
+
+      printf_filtered (_("Note: this version of macOS has System Integrity Protection.\n\
+Because `startup-with-shell' is enabled, gdb has worked around this by\n\
+caching a copy of your shell.  The shell used by \"run\" is now:\n\
+    %s\n"),
+		       new_name.c_str ());
+    }
+
+  /* We need to make sure that the new name has the correct lifetime
+     for setenv.  */
+  static std::string saved_shell = std::move (new_name);
+  copied_shell = saved_shell.c_str ();
+
+#endif /* SF_RESTRICTED */
+
+  return true;
+}
+
 void
 darwin_nat_target::create_inferior (const char *exec_file,
 				    const std::string &allargs,
@@ -1880,16 +2016,18 @@ darwin_nat_target::create_inferior (const char *exec_file,
 {
   gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
 
-  if (startup_with_shell && should_disable_startup_with_shell ())
+  if (startup_with_shell && may_have_sip ())
     {
-      warning (_("startup-with-shell not supported on this macOS version,"
-	         " disabling it."));
-      restore_startup_with_shell.emplace (&startup_with_shell, 0);
+      if (!maybe_cache_shell ())
+	{
+	  warning (_("startup-with-shell is now temporarily disabled"));
+	  restore_startup_with_shell.emplace (&startup_with_shell, 0);
+	}
     }
 
   /* Do the hard work.  */
   fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
-		 darwin_ptrace_him, darwin_pre_ptrace, NULL,
+		 darwin_ptrace_him, darwin_pre_ptrace, copied_shell,
 		 darwin_execvp);
 }
 \f
-- 
2.17.1

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

[Simon Marchi]

On 2018-09-26 7:11 a.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 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

> +    new_name.push_back ('/');
> +  new_name.append (shell);
> +
> +  /* Maybe it was cached by some earlier gdb.  */
> +  if (stat (new_name.c_str (), &sb) != 0 || !S_ISREG (sb.st_mode))
> +    {
> +      TRY
> +	{
> +	  copy_shell_to_cache (shell, new_name);
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  warning (_("This version of macOS has System Integrity Protection.\n\
> +Because `startup-with-shell' is enabled, gdb tried to work around SIP by\n\
> +caching a copy of your shell.  However, this failed:\n\
> +%s\n\
> +If you correct the problem, gdb will automatically try again the next time\n\
> +you \"run\".  To prevent these attempts, you can use:\n\
> +    set startup-with-shell off"),
> +		   ex.message);
> +	  return false;
> +	}
> +      END_CATCH
> +
> +      printf_filtered (_("Note: this version of macOS has System Integrity Protection.\n\
> +Because `startup-with-shell' is enabled, gdb has worked around this by\n\
> +caching a copy of your shell.  The shell used by \"run\" is now:\n\
> +    %s\n"),
> +		       new_name.c_str ());

I am not on Mac right now so I can't test, but I was wondering how
annoying it is to have this message every time you run and it succeeds.
I like that we explain what's happening when things go wrong, but is
it useful to explain it as well when everything works well?  Will the
user care?

Otherwise, this looks good to me, thanks!

Simon

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

[Tom Tromey]

Simon> I am not on Mac right now so I can't test, but I was wondering how
Simon> annoying it is to have this message every time you run and it succeeds.
Simon> I like that we explain what's happening when things go wrong, but is
Simon> it useful to explain it as well when everything works well?  Will the
Simon> user care?

The cache ensures that in normal operation the message is only printed once.
This happens because the message is only printed when the copy is made.
Subsequent "run"s, or even subsequent invocations of gdb, will find the
copy of the shell in the cache and remain silent.

Tom

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

[Simon Marchi]

On 2018-10-01 15:27, Tom Tromey wrote:
> Simon> I am not on Mac right now so I can't test, but I was wondering 
> how
> Simon> annoying it is to have this message every time you run and it 
> succeeds.
> Simon> I like that we explain what's happening when things go wrong, 
> but is
> Simon> it useful to explain it as well when everything works well?  
> Will the
> Simon> user care?
> 
> The cache ensures that in normal operation the message is only printed 
> once.
> This happens because the message is only printed when the copy is made.
> Subsequent "run"s, or even subsequent invocations of gdb, will find the
> copy of the shell in the cache and remain silent.
> 
> Tom

Oh, awesome then!

Simon

Re: [RFC 0/6] A different approach to startup-with-shell on macOS

[Simon Marchi]

On 2018-09-26 07:11, Tom Tromey wrote:
> One question I have is whether it's possible to build gdb on an older
> version of macOS and then run it on a newer version.  If this can be
> done, then the #if-based approach taken in the final patch will not
> work.

Good question.  I asked on #machomebrew about how they build the binary 
packages, whether a package intended for a certain macOS version is 
always built on that version of macOS, or there is some cross-version 
compiling involved.

Until we have proof that it's necessary, I think what you have done is 
fine.

> I didn't include any way to control this feature other than "set
> startup-with-shell off".  My thinking was that turning this off will
> just result in failures, which isn't useful.  However if there's a
> reason to do something else, I could add it.

Makes sense.

Simon

Re: [RFC 0/6] A different approach to startup-with-shell on macOS

[Pedro Alves]

On 09/26/2018 12:11 PM, Tom Tromey wrote:

> One question I have is whether it's possible to build gdb on an older
> version of macOS and then run it on a newer version.  If this can be
> done, then the #if-based approach taken in the final patch will not
> work.

I'd suspect so.  What, e.g., does Homebrew do?  Do they have packages
built once for every Darwin version, or a single binary for several
Darwin versions?  I'd think the latter, but I don't really know.
And if indeed the latter, do they always build on the newest
Darwin, or perhaps the oldest?

Thanks,
Pedro Alves

Re: [RFC 0/6] A different approach to startup-with-shell on macOS

[Simon Marchi]

On 2018-09-29 14:43, Pedro Alves wrote:
> On 09/26/2018 12:11 PM, Tom Tromey wrote:
> 
>> One question I have is whether it's possible to build gdb on an older
>> version of macOS and then run it on a newer version.  If this can be
>> done, then the #if-based approach taken in the final patch will not
>> work.
> 
> I'd suspect so.  What, e.g., does Homebrew do?  Do they have packages
> built once for every Darwin version, or a single binary for several
> Darwin versions?  I'd think the latter, but I don't really know.
> And if indeed the latter, do they always build on the newest
> Darwin, or perhaps the oldest?

Here's the answer I got on the homebrew IRC channel:

> homebrew generally does this (but i think it's probably more 
> conservative than it needs to be)
> a bottle should never be deployed to an older macos version than it was 
> built on, anyway

"this" refers to whether the binaries always run on the same macos 
version as the one on which they have been built.

Simon

Re: [RFC 0/6] A different approach to startup-with-shell on macOS

[Pedro Alves]

On 09/29/2018 08:50 PM, Simon Marchi wrote:
> On 2018-09-29 14:43, Pedro Alves wrote:
>> On 09/26/2018 12:11 PM, Tom Tromey wrote:
>>
>>> One question I have is whether it's possible to build gdb on an older
>>> version of macOS and then run it on a newer version.  If this can be
>>> done, then the #if-based approach taken in the final patch will not
>>> work.
>>
>> I'd suspect so.  What, e.g., does Homebrew do?  Do they have packages
>> built once for every Darwin version, or a single binary for several
>> Darwin versions?  I'd think the latter, but I don't really know.
>> And if indeed the latter, do they always build on the newest
>> Darwin, or perhaps the oldest?
> 
> Here's the answer I got on the homebrew IRC channel:
> 
>> homebrew generally does this (but i think it's probably more conservative than it needs to be)
>> a bottle should never be deployed to an older macos version than it was built on, anyway
> 
> "this" refers to whether the binaries always run on the same macos version as the one on which they have been built.

Great, that clears it up.

Thanks,
Pedro Alves

Re: [RFC 0/6] A different approach to startup-with-shell on macOS

[Tom Tromey]

>> "this" refers to whether the binaries always run on the same macos
>> version as the one on which they have been built.

Pedro> Great, that clears it up.

Homebrew doesn't need it, which I think means the current approach is
good enough for Homebrew users.  But I don't know if this is something
that should be handled by gdb anyway.  Maybe coding the constant into
gdb and doing this unconditionally is better?

Tom