diff mbox

[v3] PR 20569, segv in follow_exec

Message ID 1477346225-32400-1-git-send-email-lgustavo@codesourcery.com
State New
Headers show

Commit Message

Luis Machado Oct. 24, 2016, 9:57 p.m. UTC
Follows the updated patch with the comments addressed and a few more things
adjusted. I can't say i like the name "maybe_open_exec_file", but that was
the best that came to mind on a Monday. :-)

How does it look now?

Changes in v3:

* Addressed overal review comments (explicit NULL check, misleading variable
  name and trailing whitespaces).
* Separated bug fixes for exception_print_same and NULL dereferences. They
  were submitted as a couple separate (obvious?) patches.
* Moved duplicate code from exec_file_locate_attach and follow_exec to a
  shared function maybe_open_exec_file that gets called from both places.
* Addressed comments for the testcase, cleaned it up and made it expect a
  different output message.


Changes in v2:

* Changed exec_file_target cleanup to a exec_file_host cleanup inside
  exec.c:exec_file_locate_attach. It was a typo that caused a crash.
* Added the gdb.base/exec-invalid-sysroot.exp testcase.

As I noted in PR20569, several exec-related tests cause GDB to segv when sysroot translation fails on the executable pathname reported by gdbserver.  The immediate cause of the segv is that follow_exec is passing a NULL argument (the result of exec_file_find) to strlen, but as I looked at the code in more detail it seemed like follow_exec simply isn't prepared for the case where sysroot translation fails to locate the new executable, and there is no obvious recovery strategy.

I thought I could copy logic from the other caller of exec_file_find, exec_file_locate_attach, but I think it's doing the wrong thing there as well.  Plus, from reading the code I found other bugs in both callers of exec_file_find due to confusion between host and target pathnames.

The attached patch attempts to fix all the bugs.  In terms of the testcases that were formerly segv'ing, GDB now prints a warning but continues execution of the new program, so that the tests now mostly FAIL instead.  You could argue the FAILs are due to a legitimate problem with the test environment setting up the sysroot translation incorrectly, but I'm not sure continuing execution rather than leaving the target stopped is the most user-friendly fallback behavior, either.  E.g. the GDB manual suggests that you can set a breakpoint on main and GDB will stop on main of the newly exec'ed program, too, but it can't do that if it can't find the symbol information, and there's no way for the user to specify the executable file to GDB explicitly before it resumes execution.  But, seemed better not to complicate this already-too-complicated patch further by trying to address that in the first cut.

The following testcases will make GDB crash whenever an invalid sysroot is provided, causing GDB to be unabled to find a valid path to the symbol file.

gdb.base/catch-syscall.exp
gdb.base/execl-update-breakpoints.exp
gdb.base/foll-exec-mode.exp
gdb.base/foll-exec.exp
gdb.base/foll-vfork.exp
gdb.base/pie-execl.exp
gdb.multi/bkpt-multi-exec.exp
gdb.python/py-finish-breakpoint.exp
gdb.threads/execl.exp
gdb.threads/non-ldr-exc-1.exp
gdb.threads/non-ldr-exc-2.exp
gdb.threads/non-ldr-exc-3.exp
gdb.threads/non-ldr-exc-4.exp
gdb.threads/thread-execl.exp

gdb/ChangeLog:

2016-10-24  Sandra Loosemore  <sandra@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	PR gdb/20569
	gdb/
	* exceptions.c (exception_print_same): Moved here from exec.c.
	* exceptions.h (exception_print_same): Declare.
	* exec.h (maybe_open_exec_file): New declaration.
	* exec.c (exception_print_same): Moved to exceptions.c.
	(maybe_open_exec_file): New function.
	(exec_file_locate_attach): Rename exec_file and full_exec_path
	variables to avoid confusion between target and host pathnames.
	Move pathname processing logic to exec_file_find.  Do not return
	early if pathname lookup fails;
	Call maybe_open_exec_file.
	* infrun.c (follow_exec): Split and rename execd_pathname variable
	to avoid confusion between target and host pathnames.  Replace
	brokenpathname copy with cleanup to free malloc'ed string.  Warn
	if pathname lookup fails.  Pass target pathname to
	target_follow_exec, not hostpathname.
	Call maybe_open_exec_file.
	* solib.c (exec_file_find): Incorporate fallback logic for relative
	pathnames formerly in exec_file_locate_attach.

gdb/testsuite/ChangeLog

2016-10-24  Luis Machado  <lgustavo@codesourcery.com>

	* gdb.base/exec-invalid-sysroot.exp: New file.
---
 gdb/ChangeLog                                   |  24 +++++
 gdb/exceptions.c                                |  19 ++++
 gdb/exceptions.h                                |   3 +
 gdb/exec.c                                      | 137 ++++++++++++------------
 gdb/exec.h                                      |  10 ++
 gdb/infrun.c                                    |  41 ++++---
 gdb/solib.c                                     |  32 ++++--
 gdb/testsuite/ChangeLog                         |   4 +
 gdb/testsuite/gdb.base/exec-invalid-sysroot.exp |  69 ++++++++++++
 9 files changed, 239 insertions(+), 100 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/exec-invalid-sysroot.exp

Comments

Pedro Alves Oct. 25, 2016, 11:56 a.m. UTC | #1
On 10/24/2016 10:57 PM, Luis Machado wrote:
> Follows the updated patch with the comments addressed and a few more things
> adjusted. I can't say i like the name "maybe_open_exec_file", but that was
> the best that came to mind on a Monday. :-)

"maybe" is often used when a function first checks some condition
before doing the actual work.  This is not the case here.
The whole point of the function is actually trying to open the file,
with try/catch + warning handling.  So I'd suggest s/maybe_open_.../try_open_.../.

> 
> How does it look now?

Getting closer, but not yet, sorry.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 43175ff..0c6cc6d 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,27 @@
> +2016-10-24  Sandra Loosemore  <sandra@codesourcery.com>
> +	    Luis Machado  <lgustavo@codesourcery.com>
> +
> +	PR gdb/20569
> +	gdb/

Please don't add this "gdb/".

>  void
> -exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
> +maybe_open_exec_file (const char *exec_file_host, struct inferior *inf,
> +		      int main_symbol_file, int defer_bp_reset, int from_tty)

"main_symbol_file" and "defer_bp_reset" are both symfile_flags:
SYMFILE_MAINLINE, SYMFILE_DEFER_BP_RESET.  Seems to me that
the function could take a single symfile_add_flags parameter
instead of those two, helping avoid the "boolean trap".

>      {
> -      if (defer_bp_reset)
> -	current_inferior ()->symfile_flags |= SYMFILE_DEFER_BP_RESET;
> -      symbol_file_add_main (full_exec_path, from_tty);
> +      TRY
> +	{
> +	  if (main_symbol_file)
> +	    {
> +	      if (defer_bp_reset)
> +		inf->symfile_flags |= SYMFILE_DEFER_BP_RESET;
> +	      symbol_file_add_main (exec_file_host, from_tty);
> +	    }
> +	  else
> +	    {
> +	      symbol_file_add (exec_file_host,
> +			       (inf->symfile_flags
> +			       | SYMFILE_MAINLINE

Both then/else branches are adding a main file...  We should
be able to merge the branches.  E.g., export symbol_file_add_main_1
and call it here, passing down the new add_flags parameter that
I suggested above.

 | SYMFILE_DEFER_BP_RESET),

See, this here is not considering the defer_bp_reset argument.

> +			       NULL, 0);
> +
> +	      if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> +		set_initial_language ();
> +	    }
> +	}
> +      CATCH (err, RETURN_MASK_ERROR)
> +	{
> +	  if (!exception_print_same (prev_err, err))
> +	    warning ("%s", err.message);
> +
> +	  if (main_symbol_file)
> +	    inf->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;


And now this isn't restored on normal return.  It was, on the
current code, AFAICS.

> +	}
> +      END_CATCH
>      }
> -  CATCH (err, RETURN_MASK_ERROR)
> +
> +  do_cleanups (old_chain);
> +}
> +
> +/* See gdbcore.h.  */
> +
> +void
> +exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
> +{
> +  char *exec_file_target, *exec_file_host = NULL;

Don't need to initialize to NULL.  The existing code needs it,
because the path to initialize it may be skipped.  But that's not
the case any longer here.

> +  struct cleanup *old_chain;
> +
> +  /* Do nothing if we already have an executable filename.  */
> +  if (get_exec_file (0) != NULL)
> +    return;
> +
> +  /* Try to determine a filename from the process itself.  */
> +  exec_file_target = target_pid_to_exec_file (pid);
> +  if (exec_file_target == NULL)
>      {
> -      if (!exception_print_same (prev_err, err))
> -	warning ("%s", err.message);
> +      warning (_("No executable has been specified and target does not "
> +		 "support\n"
> +		 "determining executable automatically.  "
> +		 "Try using the \"file\" command."));
> +      return;
>      }
> -  END_CATCH
> -  current_inferior ()->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;
>  
> +  exec_file_host = exec_file_find (exec_file_target, NULL);
> +  old_chain = make_cleanup (xfree, exec_file_host);
> +
> +  /* Attempt to open the exec file.  */
> +  maybe_open_exec_file (exec_file_host, current_inferior (), 1,
> +			defer_bp_reset, from_tty);
>    do_cleanups (old_chain);
>  }
>  
> diff --git a/gdb/exec.h b/gdb/exec.h
> index 4044cb7..e893fd2 100644
> --- a/gdb/exec.h
> +++ b/gdb/exec.h
> @@ -113,4 +113,14 @@ extern void print_section_info (struct target_section_table *table,
>  
>  extern void exec_close (void);
>  
> +/* Helper function that attempts to open the symbol file at EXEC_FILE_HOST.
> +   If successful, it proceeds to add the symbol file as the main symbol file
> +   if MAIN_SYMBOL_FILE is 1.  Otherwise it attempts to add it as a regular
> +   non-main symbol file.

This is not correct.  The file is always added as a main symbol file.

> +
> +   DEFER_BP_RESET and FROM_TTY are passed on to functions called from within
> +   this helper function.  */
> +extern void maybe_open_exec_file (const char *exec_file_host,
> +				  struct inferior *inf, int main_symbol_file,
> +				  int defer_bp_reset, int from_tty);
>  #endif
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 00bba16..7d89e8e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1077,15 +1077,17 @@ show_follow_exec_mode_string (struct ui_file *file, int from_tty,
>    fprintf_filtered (file, _("Follow exec mode is \"%s\".\n"),  value);
>  }
>  
> -/* EXECD_PATHNAME is assumed to be non-NULL.  */
> +/* EXEC_FILE_TARGET is assumed to be non-NULL.  */
>  
>  static void
> -follow_exec (ptid_t ptid, char *execd_pathname)
> +follow_exec (ptid_t ptid, char *exec_file_target)
>  {
>    struct thread_info *th, *tmp;
>    struct inferior *inf = current_inferior ();
>    int pid = ptid_get_pid (ptid);
>    ptid_t process_ptid;
> +  char *exec_file_host = NULL;

No need for NULL initialization.

> +  struct cleanup *old_chain;
>  
>    /* This is an exec event that we actually wish to pay attention to.
>       Refresh our symbol table to the newly exec'd program, remove any
> @@ -1155,7 +1157,7 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>    process_ptid = pid_to_ptid (pid);
>    printf_unfiltered (_("%s is executing new program: %s\n"),
>  		     target_pid_to_str (process_ptid),
> -		     execd_pathname);
> +		     exec_file_target);
>  
>    /* We've followed the inferior through an exec.  Therefore, the
>       inferior has essentially been killed & reborn.  */
> @@ -1164,14 +1166,17 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>    breakpoint_init_inferior (inf_execd);
>  
> -  if (*gdb_sysroot != '\0')
> -    {
> -      char *name = exec_file_find (execd_pathname, NULL);
> +  exec_file_host = exec_file_find (exec_file_target, NULL);
> +  old_chain = make_cleanup (xfree, exec_file_host);
>  
> -      execd_pathname = (char *) alloca (strlen (name) + 1);
> -      strcpy (execd_pathname, name);
> -      xfree (name);
> -    }
> +  /* If we were unable to map the executable target pathname onto a host
> +     pathname, tell the user that.  Otherwise GDB's subsequent behavior
> +     is confusing.  Maybe it would even be better to stop at this point
> +     so that the user can specify a file manually before continuing.  */
> +  if (exec_file_host == NULL)
> +    warning (_("Could not load symbols for executable %s.\n"
> +	       "Do you need \"set sysroot\"?"),
> +	     exec_file_target);
>  
>    /* Reset the shared library package.  This ensures that we get a
>       shlib event when the child reaches "_start", at which point the
> @@ -1193,7 +1198,7 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>        inf = add_inferior_with_spaces ();
>        inf->pid = pid;
> -      target_follow_exec (inf, execd_pathname);
> +      target_follow_exec (inf, exec_file_target);
>  
>        set_current_inferior (inf);
>        set_current_program_space (inf->pspace);
> @@ -1212,22 +1217,16 @@ follow_exec (ptid_t ptid, char *execd_pathname)
>  
>    gdb_assert (current_program_space == inf->pspace);
>  
> -  /* That a.out is now the one to use.  */
> -  exec_file_attach (execd_pathname, 0);
> +  /* Attempt to open the exec file.  */
> +  maybe_open_exec_file (exec_file_host, inf, 0, 0, 0);
> +
> +  do_cleanups (old_chain);
>  
>    /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE
>       (Position Independent Executable) main symbol file will get applied by
>       solib_create_inferior_hook below.  breakpoint_re_set would fail to insert
>       the breakpoints with the zero displacement.  */

This comment needs to move somewhere.  Here it will feel very lonely
and out of place.  :-)


>  
> -  symbol_file_add (execd_pathname,
> -		   (inf->symfile_flags
> -		    | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET),
> -		   NULL, 0);
> -
> -  if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
> -    set_initial_language ();
> -
>    /* If the target can specify a description, read it.  Must do this
>       after flipping to the new executable (because the target supplied
>       description must be compatible with the executable's
> diff --git a/gdb/solib.c b/gdb/solib.c
> index b8c2b42..db0ef02 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -380,21 +380,22 @@ solib_find_1 (char *in_pathname, int *fd, int is_solib)
>  /* Return the full pathname of the main executable, or NULL if not
>     found.  The returned pathname is malloc'ed and must be freed by
>     the caller.  If FD is non-NULL, *FD is set to either -1 or an open
> -   file handle for the main executable.
> -
> -   The search algorithm used is described in solib_find_1's comment
> -   above.  */
> +   file handle for the main executable.  */
>  
>  char *
>  exec_file_find (char *in_pathname, int *fd)
>  {
> -  char *result = solib_find_1 (in_pathname, fd, 0);
> +  char *result;
> +  const char *fskind = effective_target_file_system_kind ();
> +
> +  if (!in_pathname)

in_pathname == NULL

> +    return NULL;

> +
> +# This test exercises PR20569.  GDB crashes when attempting to follow
> +# an exec call when it cannot resolve the path to the symbol file.
> +# This is the case when an invalid sysroot is provided.

I think it'd be better to write this in the past tense.  GDB no longer
crashes after the fix.  would crash...could not...was... etc.

> +
> +standard_testfile foll-exec.c
> +
> +global binfile
> +set binfile [standard_output_file "foll-exec"]
> +set testfile2 "execd-prog"
> +set srcfile2 ${testfile2}.c
> +set binfile2 [standard_output_file ${testfile2}]
> +
> +set compile_options debug
> +
> +# build the first test case
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable $compile_options] != "" } {
> +    untested foll-exec.exp

Wrong .exp name.  But:
  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls

> +    return -1
> +}
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_options] != "" } {
> +    untested foll-exec.exp

Ditto.

> +    return -1
> +}
> +
> +proc do_exec_sysroot_test {} {
> +    global binfile srcfile srcfile2 testfile testfile2
> +    global gdb_prompt
> +
> +    # Start the program running, and stop at main.
> +    #
> +    if ![runto_main] then {
> +	fail "Couldn't run ${testfile}"
> +	return
> +    }
> +
> +    # Verify that the system supports "catch exec".
> +    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
> +    set test "continue to first exec catchpoint"

s/first// while at it.  There's only one.

> +    gdb_test_multiple "continue" $test {
> +	-re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
> +	    unsupported $test
> +	    return
> +	}
> +	-re ".*Could not load symbols for executable.*$gdb_prompt $" {
> +	    pass $test
> +	}
> +    }
> +}
> +
> +# Start with a fresh gdb
> +clean_restart $binfile
> +gdb_test_no_output "set sysroot /a/path/that/does/not/exist"

Odd to put this here instead of inside the sysroot_test function, IMO.

> +do_exec_sysroot_test

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 43175ff..0c6cc6d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,27 @@ 
+2016-10-24  Sandra Loosemore  <sandra@codesourcery.com>
+	    Luis Machado  <lgustavo@codesourcery.com>
+
+	PR gdb/20569
+	gdb/
+	* exceptions.c (exception_print_same): Moved here from exec.c.
+	* exceptions.h (exception_print_same): Declare.
+	* exec.h (maybe_open_exec_file): New declaration.
+	* exec.c (exception_print_same): Moved to exceptions.c.
+	(maybe_open_exec_file): New function.
+	(exec_file_locate_attach): Rename exec_file and full_exec_path
+	variables to avoid confusion between target and host pathnames.
+	Move pathname processing logic to exec_file_find.  Do not return
+	early if pathname lookup fails;
+	Call maybe_open_exec_file.
+	* infrun.c (follow_exec): Split and rename execd_pathname variable
+	to avoid confusion between target and host pathnames.  Replace
+	brokenpathname copy with cleanup to free malloc'ed string.  Warn
+	if pathname lookup fails.  Pass target pathname to
+	target_follow_exec, not hostpathname.
+	Call maybe_open_exec_file.
+	* solib.c (exec_file_find): Incorporate fallback logic for relative
+	pathnames formerly in exec_file_locate_attach.
+
 2016-10-24  Luis Machado  <lgustavo@codesourcery.com>
 
 	* exec.c (exec_file_locate_attach): Prevent NULL pointer dereference
diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index 9a10f66..0e269de 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -256,3 +256,22 @@  catch_errors (catch_errors_ftype *func, void *func_args, char *errstring,
     return 0;
   return val;
 }
+
+/* Returns non-zero if exceptions E1 and E2 are equal.  Returns zero
+   otherwise.  */
+
+int
+exception_print_same (struct gdb_exception e1, struct gdb_exception e2)
+{
+  const char *msg1 = e1.message;
+  const char *msg2 = e2.message;
+
+  if (msg1 == NULL)
+    msg1 = "";
+  if (msg2 == NULL)
+    msg2 = "";
+
+  return (e1.reason == e2.reason
+	  && e1.error == e2.error
+	  && strcmp (msg1, msg2) == 0);
+}
diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 5711c1a..a2d39d7 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -88,4 +88,7 @@  extern int catch_exceptions_with_msg (struct ui_out *uiout,
 typedef int (catch_errors_ftype) (void *);
 extern int catch_errors (catch_errors_ftype *, void *, char *, return_mask);
 
+/* Compare two exception objects for print equality.  */
+extern int exception_print_same (struct gdb_exception e1,
+				 struct gdb_exception e2);
 #endif
diff --git a/gdb/exec.c b/gdb/exec.c
index 5eeac44..c4806e3 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -136,73 +136,16 @@  exec_file_clear (int from_tty)
     printf_unfiltered (_("No executable file now.\n"));
 }
 
-/* Returns non-zero if exceptions E1 and E2 are equal.  Returns zero
-   otherwise.  */
-
-static int
-exception_print_same (struct gdb_exception e1, struct gdb_exception e2)
-{
-  const char *msg1 = e1.message;
-  const char *msg2 = e2.message;
-
-  if (msg1 == NULL)
-    msg1 = "";
-  if (msg2 == NULL)
-    msg2 = "";
-
-  return (e1.reason == e2.reason
-	  && e1.error == e2.error
-	  && strcmp (msg1, msg2) == 0);
-}
-
-/* See gdbcore.h.  */
+/* See exec.h.  */
 
 void
-exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
+maybe_open_exec_file (const char *exec_file_host, struct inferior *inf,
+		      int main_symbol_file, int defer_bp_reset, int from_tty)
 {
-  char *exec_file, *full_exec_path = NULL;
   struct cleanup *old_chain;
   struct gdb_exception prev_err = exception_none;
 
-  /* Do nothing if we already have an executable filename.  */
-  exec_file = (char *) get_exec_file (0);
-  if (exec_file != NULL)
-    return;
-
-  /* Try to determine a filename from the process itself.  */
-  exec_file = target_pid_to_exec_file (pid);
-  if (exec_file == NULL)
-    {
-      warning (_("No executable has been specified and target does not "
-		 "support\n"
-		 "determining executable automatically.  "
-		 "Try using the \"file\" command."));
-      return;
-    }
-
-  /* If gdb_sysroot is not empty and the discovered filename
-     is absolute then prefix the filename with gdb_sysroot.  */
-  if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file))
-    {
-      full_exec_path = exec_file_find (exec_file, NULL);
-      if (full_exec_path == NULL)
-	return;
-    }
-  else
-    {
-      /* It's possible we don't have a full path, but rather just a
-	 filename.  Some targets, such as HP-UX, don't provide the
-	 full path, sigh.
-
-	 Attempt to qualify the filename against the source path.
-	 (If that fails, we'll just fall back on the original
-	 filename.  Not much more we can do...)  */
-      if (!source_full_path_of (exec_file, &full_exec_path))
-	full_exec_path = xstrdup (exec_file);
-    }
-
-  old_chain = make_cleanup (xfree, full_exec_path);
-  make_cleanup (free_current_contents, &prev_err.message);
+  old_chain = make_cleanup (free_current_contents, &prev_err.message);
 
   /* exec_file_attach and symbol_file_add_main may throw an error if the file
      cannot be opened either locally or remotely.
@@ -217,7 +160,9 @@  exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
      errors/exceptions in the following code.  */
   TRY
     {
-      exec_file_attach (full_exec_path, from_tty);
+      /* We must do this step even if exec_file_host is NULL, so that
+	 exec_file_attach will clear state.  */
+      exec_file_attach (exec_file_host, from_tty);
     }
   CATCH (err, RETURN_MASK_ERROR)
     {
@@ -238,20 +183,70 @@  exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
     }
   END_CATCH
 
-  TRY
+  if (exec_file_host != NULL)
     {
-      if (defer_bp_reset)
-	current_inferior ()->symfile_flags |= SYMFILE_DEFER_BP_RESET;
-      symbol_file_add_main (full_exec_path, from_tty);
+      TRY
+	{
+	  if (main_symbol_file)
+	    {
+	      if (defer_bp_reset)
+		inf->symfile_flags |= SYMFILE_DEFER_BP_RESET;
+	      symbol_file_add_main (exec_file_host, from_tty);
+	    }
+	  else
+	    {
+	      symbol_file_add (exec_file_host,
+			       (inf->symfile_flags
+			       | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET),
+			       NULL, 0);
+
+	      if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
+		set_initial_language ();
+	    }
+	}
+      CATCH (err, RETURN_MASK_ERROR)
+	{
+	  if (!exception_print_same (prev_err, err))
+	    warning ("%s", err.message);
+
+	  if (main_symbol_file)
+	    inf->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;
+	}
+      END_CATCH
     }
-  CATCH (err, RETURN_MASK_ERROR)
+
+  do_cleanups (old_chain);
+}
+
+/* See gdbcore.h.  */
+
+void
+exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty)
+{
+  char *exec_file_target, *exec_file_host = NULL;
+  struct cleanup *old_chain;
+
+  /* Do nothing if we already have an executable filename.  */
+  if (get_exec_file (0) != NULL)
+    return;
+
+  /* Try to determine a filename from the process itself.  */
+  exec_file_target = target_pid_to_exec_file (pid);
+  if (exec_file_target == NULL)
     {
-      if (!exception_print_same (prev_err, err))
-	warning ("%s", err.message);
+      warning (_("No executable has been specified and target does not "
+		 "support\n"
+		 "determining executable automatically.  "
+		 "Try using the \"file\" command."));
+      return;
     }
-  END_CATCH
-  current_inferior ()->symfile_flags &= ~SYMFILE_DEFER_BP_RESET;
 
+  exec_file_host = exec_file_find (exec_file_target, NULL);
+  old_chain = make_cleanup (xfree, exec_file_host);
+
+  /* Attempt to open the exec file.  */
+  maybe_open_exec_file (exec_file_host, current_inferior (), 1,
+			defer_bp_reset, from_tty);
   do_cleanups (old_chain);
 }
 
diff --git a/gdb/exec.h b/gdb/exec.h
index 4044cb7..e893fd2 100644
--- a/gdb/exec.h
+++ b/gdb/exec.h
@@ -113,4 +113,14 @@  extern void print_section_info (struct target_section_table *table,
 
 extern void exec_close (void);
 
+/* Helper function that attempts to open the symbol file at EXEC_FILE_HOST.
+   If successful, it proceeds to add the symbol file as the main symbol file
+   if MAIN_SYMBOL_FILE is 1.  Otherwise it attempts to add it as a regular
+   non-main symbol file.
+
+   DEFER_BP_RESET and FROM_TTY are passed on to functions called from within
+   this helper function.  */
+extern void maybe_open_exec_file (const char *exec_file_host,
+				  struct inferior *inf, int main_symbol_file,
+				  int defer_bp_reset, int from_tty);
 #endif
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 00bba16..7d89e8e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1077,15 +1077,17 @@  show_follow_exec_mode_string (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("Follow exec mode is \"%s\".\n"),  value);
 }
 
-/* EXECD_PATHNAME is assumed to be non-NULL.  */
+/* EXEC_FILE_TARGET is assumed to be non-NULL.  */
 
 static void
-follow_exec (ptid_t ptid, char *execd_pathname)
+follow_exec (ptid_t ptid, char *exec_file_target)
 {
   struct thread_info *th, *tmp;
   struct inferior *inf = current_inferior ();
   int pid = ptid_get_pid (ptid);
   ptid_t process_ptid;
+  char *exec_file_host = NULL;
+  struct cleanup *old_chain;
 
   /* This is an exec event that we actually wish to pay attention to.
      Refresh our symbol table to the newly exec'd program, remove any
@@ -1155,7 +1157,7 @@  follow_exec (ptid_t ptid, char *execd_pathname)
   process_ptid = pid_to_ptid (pid);
   printf_unfiltered (_("%s is executing new program: %s\n"),
 		     target_pid_to_str (process_ptid),
-		     execd_pathname);
+		     exec_file_target);
 
   /* We've followed the inferior through an exec.  Therefore, the
      inferior has essentially been killed & reborn.  */
@@ -1164,14 +1166,17 @@  follow_exec (ptid_t ptid, char *execd_pathname)
 
   breakpoint_init_inferior (inf_execd);
 
-  if (*gdb_sysroot != '\0')
-    {
-      char *name = exec_file_find (execd_pathname, NULL);
+  exec_file_host = exec_file_find (exec_file_target, NULL);
+  old_chain = make_cleanup (xfree, exec_file_host);
 
-      execd_pathname = (char *) alloca (strlen (name) + 1);
-      strcpy (execd_pathname, name);
-      xfree (name);
-    }
+  /* If we were unable to map the executable target pathname onto a host
+     pathname, tell the user that.  Otherwise GDB's subsequent behavior
+     is confusing.  Maybe it would even be better to stop at this point
+     so that the user can specify a file manually before continuing.  */
+  if (exec_file_host == NULL)
+    warning (_("Could not load symbols for executable %s.\n"
+	       "Do you need \"set sysroot\"?"),
+	     exec_file_target);
 
   /* Reset the shared library package.  This ensures that we get a
      shlib event when the child reaches "_start", at which point the
@@ -1193,7 +1198,7 @@  follow_exec (ptid_t ptid, char *execd_pathname)
 
       inf = add_inferior_with_spaces ();
       inf->pid = pid;
-      target_follow_exec (inf, execd_pathname);
+      target_follow_exec (inf, exec_file_target);
 
       set_current_inferior (inf);
       set_current_program_space (inf->pspace);
@@ -1212,22 +1217,16 @@  follow_exec (ptid_t ptid, char *execd_pathname)
 
   gdb_assert (current_program_space == inf->pspace);
 
-  /* That a.out is now the one to use.  */
-  exec_file_attach (execd_pathname, 0);
+  /* Attempt to open the exec file.  */
+  maybe_open_exec_file (exec_file_host, inf, 0, 0, 0);
+
+  do_cleanups (old_chain);
 
   /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE
      (Position Independent Executable) main symbol file will get applied by
      solib_create_inferior_hook below.  breakpoint_re_set would fail to insert
      the breakpoints with the zero displacement.  */
 
-  symbol_file_add (execd_pathname,
-		   (inf->symfile_flags
-		    | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET),
-		   NULL, 0);
-
-  if ((inf->symfile_flags & SYMFILE_NO_READ) == 0)
-    set_initial_language ();
-
   /* If the target can specify a description, read it.  Must do this
      after flipping to the new executable (because the target supplied
      description must be compatible with the executable's
diff --git a/gdb/solib.c b/gdb/solib.c
index b8c2b42..db0ef02 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -380,21 +380,22 @@  solib_find_1 (char *in_pathname, int *fd, int is_solib)
 /* Return the full pathname of the main executable, or NULL if not
    found.  The returned pathname is malloc'ed and must be freed by
    the caller.  If FD is non-NULL, *FD is set to either -1 or an open
-   file handle for the main executable.
-
-   The search algorithm used is described in solib_find_1's comment
-   above.  */
+   file handle for the main executable.  */
 
 char *
 exec_file_find (char *in_pathname, int *fd)
 {
-  char *result = solib_find_1 (in_pathname, fd, 0);
+  char *result;
+  const char *fskind = effective_target_file_system_kind ();
+
+  if (!in_pathname)
+    return NULL;
 
-  if (result == NULL)
+  if (*gdb_sysroot != '\0' && IS_TARGET_ABSOLUTE_PATH (fskind, in_pathname))
     {
-      const char *fskind = effective_target_file_system_kind ();
+      result = solib_find_1 (in_pathname, fd, 0);
 
-      if (fskind == file_system_kind_dos_based)
+      if (result == NULL && fskind == file_system_kind_dos_based)
 	{
 	  char *new_pathname;
 
@@ -405,6 +406,21 @@  exec_file_find (char *in_pathname, int *fd)
 	  result = solib_find_1 (new_pathname, fd, 0);
 	}
     }
+  else
+    {
+      /* It's possible we don't have a full path, but rather just a
+	 filename.  Some targets, such as HP-UX, don't provide the
+	 full path, sigh.
+
+	 Attempt to qualify the filename against the source path.
+	 (If that fails, we'll just fall back on the original
+	 filename.  Not much more we can do...)  */
+
+      if (!source_full_path_of (in_pathname, &result))
+	result = xstrdup (in_pathname);
+      if (fd != NULL)
+	*fd = -1;
+    }
 
   return result;
 }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 460e6b9..33e5ace 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@ 
+2016-10-24  Luis Machado  <lgustavo@codesourcery.com>
+
+	* gdb.base/exec-invalid-sysroot.exp: New file.
+
 2016-10-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.base/morestack.exp: Try to build it using -fuse-ld=gold first.
diff --git a/gdb/testsuite/gdb.base/exec-invalid-sysroot.exp b/gdb/testsuite/gdb.base/exec-invalid-sysroot.exp
new file mode 100644
index 0000000..226350f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/exec-invalid-sysroot.exp
@@ -0,0 +1,69 @@ 
+#   Copyright 1997-2016 Free Software Foundation, Inc.
+
+# 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/>.
+
+# This test exercises PR20569.  GDB crashes when attempting to follow
+# an exec call when it cannot resolve the path to the symbol file.
+# This is the case when an invalid sysroot is provided.
+
+standard_testfile foll-exec.c
+
+global binfile
+set binfile [standard_output_file "foll-exec"]
+set testfile2 "execd-prog"
+set srcfile2 ${testfile2}.c
+set binfile2 [standard_output_file ${testfile2}]
+
+set compile_options debug
+
+# build the first test case
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable $compile_options] != "" } {
+    untested foll-exec.exp
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_options] != "" } {
+    untested foll-exec.exp
+    return -1
+}
+
+proc do_exec_sysroot_test {} {
+    global binfile srcfile srcfile2 testfile testfile2
+    global gdb_prompt
+
+    # Start the program running, and stop at main.
+    #
+    if ![runto_main] then {
+	fail "Couldn't run ${testfile}"
+	return
+    }
+
+    # Verify that the system supports "catch exec".
+    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
+    set test "continue to first exec catchpoint"
+    gdb_test_multiple "continue" $test {
+	-re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
+	    unsupported $test
+	    return
+	}
+	-re ".*Could not load symbols for executable.*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+}
+
+# Start with a fresh gdb
+clean_restart $binfile
+gdb_test_no_output "set sysroot /a/path/that/does/not/exist"
+do_exec_sysroot_test