Re: [PATCH] Fix for PR breakpoints/16392: info prog after failure to insert breakpoint can cause gdb crash

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Sunday, January 19 2014, I wrote:

> Hi,

Ping.

> This patch fixes PR breakpoints/16392.  The failure happens when one
> inserts a breakpoint at an invalid address, runs the inferior (thus
> receiving a warning by GDB), and then issues a "info program" command
> after the warning.
>
> What happens when the user runs the inferior is:
>
> proceed -> insert_breakpoints -> insert_breakpoint_locations
>
> And the error actually happens on insert_breakpoint_locations.  When it
> notices that the breakpoint failed to be inserted, it calls error_stream
> (in order to print the warning), which in turn calls error and aborts
> the proceed call.  This leaves GDB in a bad state, because there was
> much more thing to be done after the breakpoint insertion.
>
> I had some alternatives to deal with this.  The first one was to somehow
> give up proceeding the inferior at all, and maybe kill it.  I didn't
> like this approach because I thought it would be a drastic solution to a
> not-so-urgent problem.  Another alternative would be to just call
> warning from insert_breakpoint_locations (instead of error; I actually
> had a patch implementing a new "warning_stream" function), and then stop
> throwing the error.  However, other functions are tailored expecting an
> error to be thrown from that function (more on that later).  Therefore,
> the last alternative seemed like the better one: to handle the exception
> from proceed, and continue the function even if there is an error
> inserting the breakpoint.
>
> I implemented the latter, and noticed that GDB was still stopping after
> it noticed that the breakpoint was invalid.  I was kind of surprised
> because I was expecting the inferior to continue running, which would
> force the user to do a C-c if she wanted to issue a "info prog".  Also,
> when I first implemented the "warning_stream" idea, the inferior
> actually continued running...  Well, time for a little more
> investigation.
>
> I noticed that keep_going also calls insert_breakpoints, but that it
> actually *checks* for the exception, and doesn't keep going if there was
> some error.  Therefore, the modus operandi for GDB doesn't change with
> this patch (i.e., the inferior will still be stopped if the breakpoint
> is invalid), but now the user can inspect the program with "info prog".
>
> I hope I managed to cover everything here.  This patch also includes a
> testcase for the bug, and does not introduce any regressions on x86_64
> Fedora 17.
>
> OK to apply?
>
> -- 
> Sergio
>
> gdb/
> 2014-01-19  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	PR breakpoints/16392
> 	* infrun.c (proceed): Catch the exception from insert_breakpoints.
>
> gdb/testsuite/
> 2014-01-19  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	PR breakpoints/16392
> 	* gdb.base/break-invalid-addr-info-prog.exp: New file.
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 51540b3..e91e8c0 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2247,7 +2247,22 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
>       or if we are stepping over one but we're using displaced stepping
>       to do so.  */
>    if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
> -    insert_breakpoints ();
> +    {
> +      volatile struct gdb_exception ex;
> +
> +      /* We are interested in catching this exception (thrown by
> +	 insert_breakpoint_locations), but we will not make anything
> +	 with it, because we want to continue with the proceed.  If we
> +	 stop now, and we are dealing with the initialization of the
> +	 inferior, then GDB will be end in a bad internal state and
> +	 will not be able to handle the inferior later (see PR
> +	 breakpoints/16392). This exception will be caught again later
> +	 on keep_going, and then it will do the right thing there.  */
> +      TRY_CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  insert_breakpoints ();
> +	}
> +    }
>  
>    if (!non_stop)
>      {
> diff --git a/gdb/testsuite/gdb.base/break-invalid-addr-info-prog.exp b/gdb/testsuite/gdb.base/break-invalid-addr-info-prog.exp
> new file mode 100644
> index 0000000..0b0bcf6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/break-invalid-addr-info-prog.exp
> @@ -0,0 +1,33 @@
> +# Copyright 2014 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/>.
> +
> +# Test if, after setting a breakpoint in an invalid address in the
> +# inferior and running it (thus causing GDB to display the error), the
> +# command "info program" works OK (see PR breakpoints/16392).
> +
> +standard_testfile normal.c
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } then {
> +    return -1
> +}
> +
> +gdb_test "break *0xdeadbeef" "Breakpoint 1 at 0xdeadbeef" \
> +    "Break at invalid address"
> +
> +gdb_test "run" "Starting program: .*\[\r\n\]+Warning:\[\r\n\]+Cannot insert breakpoint 1\.\[\r\n\]+Cannot access memory at address 0xdeadbeef.*" \
> +    "Running the inferior"
> +
> +gdb_test "info program" "\tUsing the running image of child process $decimal\.\[\r\n\]+Program stopped at $hex\.\[\r\n\]+It stopped with signal SIGTRAP, Trace/breakpoint trap." \
> +    "info program works"

-- 
Sergio