Re: [rfc] PR 20569, segv in follow_exec

[Luis Machado <lgustavo@codesourcery.com>]

On 10/06/2016 05:51 PM, Sandra Loosemore wrote:
> 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.
>
> Comments?  Suggestions?  Etc.
>
> -Sandra
>
>
> gdb-segv2.log
>
>
> 2016-10-06  Sandra Loosemore  <sandra@codesourcery.com>
>
> 	PR gdb/20569
> 	gdb/
> 	* exceptions.c (exception_print_same): Moved here from exec.c.
> 	Fixed message comparison.
> 	* exceptions.h (exception_print_same): Declare.
> 	* exec_file_locate_attach (exception_print_same): Delete copy here.
> 	(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;	guard symbol_file_add_main call
> 	instead.
> 	* 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.  Borrow exception-handling
> 	logic from exec_file_locate_attach.
> 	* solib.c (exec_file_find): Incorporate fallback logic for relative
> 	pathnames formerly in exec_file_locate_attach.
>
>
> gdb-segv2.patch
>
>

I went through the patch and, although this code as a whole is a bit on 
the convoluted side, it looks reasonable to me.

Segfaults are not supposed to happen, so allowing the session to 
continue is a good thing IMO.

Sounds like a good candidate for master and even stable branches.