[RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Philippe Waroquiers]

This crash was detected when using GDB with the valgrind gdbserver.
To reproduce:

valgrind sleep 10000

In another window:
gdb
target remote | vgdb
p printf("make sleep print something\n")
=>
terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR'
Aborted

The problem is that the valgrind gdbserver does not allow to change
registers when the inferior is blocked in a system call.
GDB then raises an exception.  The exception causes the destructor
of
 typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter>
    infcall_suspend_state_up;
to be called.  This destructor itself tries to restore the value of
the registers, and fails similarly.  We must catch the exception in
the destructor to avoid crashing GDB.
If the destructor encounters a problem, no warning is produced if
there is an uncaught exception, as in this case, the user will already
be informed of a problem via this exception.

With this change, no crash anymore, and all the valgrind 3.15 tests
pass succesfully.

Note: when this patch is approved, I will push an equivalent patch
on master, but with TRY/CATCH/e.message () replaced by
try/catch/e.what ().

gdb/ChangeLog

struct infcall_suspend_state_deleter
2019-04-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* inferior.h (struct infcall_suspend_state_deleter):
	Catch exception in destructor to avoid crash.
---
 gdb/inferior.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index 2d1bb97a28..4d84afac6a 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -68,7 +68,19 @@ struct infcall_suspend_state_deleter
 {
   void operator() (struct infcall_suspend_state *state) const
   {
-    restore_infcall_suspend_state (state);
+    TRY
+      {
+	restore_infcall_suspend_state (state);
+      }
+    CATCH (e, RETURN_MASK_ALL)
+      {
+	/* If we are restoring the inferior state due to an exception,
+	   some error message will be printed.  So, only warn the user
+	   when we cannot restore during normal execution.  */
+	if (!std::uncaught_exception ())
+	  warning (_("Failed to restore inferior state: %s"), e.message);
+      }
+    END_CATCH
   }
 };
 
-- 
2.20.1

Re: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Mark Wielaard]

Hi,

On Sat, Apr 13, 2019 at 10:28:17AM +0200, Philippe Waroquiers wrote:
> This crash was detected when using GDB with the valgrind gdbserver.
> To reproduce:
> 
> valgrind sleep 10000
> 
> In another window:
> gdb
> target remote | vgdb
> p printf("make sleep print something\n")
> =>
> terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR'
> Aborted
> 
> The problem is that the valgrind gdbserver does not allow to change
> registers when the inferior is blocked in a system call.
> GDB then raises an exception.  The exception causes the destructor
> of
>  typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter>
>     infcall_suspend_state_up;
> to be called.  This destructor itself tries to restore the value of
> the registers, and fails similarly.  We must catch the exception in
> the destructor to avoid crashing GDB.
> If the destructor encounters a problem, no warning is produced if
> there is an uncaught exception, as in this case, the user will already
> be informed of a problem via this exception.
> 
> With this change, no crash anymore, and all the valgrind 3.15 tests
> pass succesfully.
> 
> Note: when this patch is approved, I will push an equivalent patch
> on master, but with TRY/CATCH/e.message () replaced by
> try/catch/e.what ().
> 
> gdb/ChangeLog
> 
> struct infcall_suspend_state_deleter
> 2019-04-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* inferior.h (struct infcall_suspend_state_deleter):
> 	Catch exception in destructor to avoid crash.

What is the current status of this patch?
We really need something like this to enable the valgrind gdbserver
integration testing again on Fedora 30/rawhide.

Thanks,

Mark

> ---
>  gdb/inferior.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 2d1bb97a28..4d84afac6a 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -68,7 +68,19 @@ struct infcall_suspend_state_deleter
>  {
>    void operator() (struct infcall_suspend_state *state) const
>    {
> -    restore_infcall_suspend_state (state);
> +    TRY
> +      {
> +	restore_infcall_suspend_state (state);
> +      }
> +    CATCH (e, RETURN_MASK_ALL)
> +      {
> +	/* If we are restoring the inferior state due to an exception,
> +	   some error message will be printed.  So, only warn the user
> +	   when we cannot restore during normal execution.  */
> +	if (!std::uncaught_exception ())
> +	  warning (_("Failed to restore inferior state: %s"), e.message);
> +      }
> +    END_CATCH
>    }
>  };
>  
> -- 
> 2.20.1

Re: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Philippe Waroquiers]

On Fri, 2019-04-19 at 13:03 +0200, Mark Wielaard wrote:
> > gdb/ChangeLog
> > 
> > struct infcall_suspend_state_deleter
> > 2019-04-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> > 
> > 	* inferior.h (struct infcall_suspend_state_deleter):
> > 	Catch exception in destructor to avoid crash.
> 
> What is the current status of this patch?
> We really need something like this to enable the valgrind gdbserver
> integration testing again on Fedora 30/rawhide.
This is still in state RFA, waiting for review.
Philippe

Re: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Pedro Alves]

On 4/19/19 12:36 PM, Philippe Waroquiers wrote:
> On Fri, 2019-04-19 at 13:03 +0200, Mark Wielaard wrote:
>>> gdb/ChangeLog
>>>
>>> struct infcall_suspend_state_deleter
>>> 2019-04-13  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>>>
>>> 	* inferior.h (struct infcall_suspend_state_deleter):
>>> 	Catch exception in destructor to avoid crash.
>>
>> What is the current status of this patch?
>> We really need something like this to enable the valgrind gdbserver
>> integration testing again on Fedora 30/rawhide.
> This is still in state RFA, waiting for review.

I had not realized you had posted this updated patch.

This version is OK.

Thanks,
Pedro Alves

Re: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Philippe Waroquiers]

On Fri, 2019-04-19 at 12:48 +0100, Pedro Alves wrote:
> This version is OK.
Thanks for the review and the suggestion to use
std::uncaught_exception ().

Pushed to 8.3 branch.
Pushed an equivalent patch to master,
  with TRY/CATCH/e.message replaced by try/catch/e.what ().

Philippe

Re: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified.

[Sergio Durigan Junior]

On Friday, April 19 2019, Philippe Waroquiers wrote:

> On Fri, 2019-04-19 at 12:48 +0100, Pedro Alves wrote:
>> This version is OK.
> Thanks for the review and the suggestion to use
> std::uncaught_exception ().
>
> Pushed to 8.3 branch.
> Pushed an equivalent patch to master,
>   with TRY/CATCH/e.message replaced by try/catch/e.what ().

Today is a holiday here, but I'll rebase F30 GDB first thing tomorrow.
Thanks.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/