[PATCH 0/1] Crash on register invalidation

[PATCH 0/1] Crash on register invalidation

[Eduard Sargsyan]

From: "Sargsyan, Eduard" <eduard.sargsyan@intel.com>

Hi all,

There was an assertion triggered, when gdb tries to invalidate register
cache after gdbserver connection is not dead. I beleive in such cases we
do not need asssertion.

Regards,
Eduard

Eduard Sargsyan (1):
  gdb: fix assert after register write failure

 gdb/regcache.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 1/1] gdb: fix assert after register write failure

[Eduard Sargsyan]

GDB tries to invalidate register cache if write to register fails.  In case
when write was requested to be done for a remote inferior and connection with
remote is dead (so write request will fail) GDB cleans register caches and as
result setting register status fails with segmentation fault, because internal
pointers are not valid anymore and the unique_ptr with register status is
null.

This change adds check if register status cache is still valid before trying to
modify (set status to UNKNOWN).

gdb/ChangeLog:
2021-03-18  Eduard Sargsyan  <eduard.sargsyan@intel.com>

	* regcache.c (regcache::raw_write): Add check for pointer validity.
---
 gdb/regcache.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 6140a05f02b..94ca89a58de 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -882,8 +882,11 @@ regcache::raw_write (int regnum, gdb::array_view<const gdb_byte> src)
 
   /* Invalidate the register after it is written, in case of a
      failure.  */
-  auto invalidator
-    = make_scope_exit ([&] { this->invalidate (regnum); });
+  auto invalidator = make_scope_exit ([&]
+    {
+      if (this->m_register_status != nullptr)
+	this->invalidate (regnum);
+    });
 
   target_store_registers (this, regnum);
 
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] gdb: fix assert after register write failure

[Tom Tromey]

>>>>> Eduard Sargsyan <eduard.sargsyan@intel.com> writes:

> GDB tries to invalidate register cache if write to register fails.  In case
> when write was requested to be done for a remote inferior and connection with
> remote is dead (so write request will fail) GDB cleans register caches and as
> result setting register status fails with segmentation fault, because internal
> pointers are not valid anymore and the unique_ptr with register status is
> null.

> This change adds check if register status cache is still valid before trying to
> modify (set status to UNKNOWN).

Could you say how to reproduce this?  If it's an existing test case,
then that's fine; but I am wondering if it needs a new test.

Tom

Re: [PATCH 1/1] gdb: fix assert after register write failure

[Simon Marchi]

On 2023-12-19 10:14, Eduard Sargsyan wrote:
> GDB tries to invalidate register cache if write to register fails.  In case
> when write was requested to be done for a remote inferior and connection with
> remote is dead (so write request will fail) GDB cleans register caches and as
> result setting register status fails with segmentation fault, because internal
> pointers are not valid anymore and the unique_ptr with register status is
> null.
> 
> This change adds check if register status cache is still valid before trying to
> modify (set status to UNKNOWN).
> 
> gdb/ChangeLog:
> 2021-03-18  Eduard Sargsyan  <eduard.sargsyan@intel.com>
> 
> 	* regcache.c (regcache::raw_write): Add check for pointer validity.
> ---
>  gdb/regcache.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 6140a05f02b..94ca89a58de 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -882,8 +882,11 @@ regcache::raw_write (int regnum, gdb::array_view<const gdb_byte> src)
>  
>    /* Invalidate the register after it is written, in case of a
>       failure.  */
> -  auto invalidator
> -    = make_scope_exit ([&] { this->invalidate (regnum); });
> +  auto invalidator = make_scope_exit ([&]
> +    {
> +      if (this->m_register_status != nullptr)
> +	this->invalidate (regnum);
> +    });
>  
>    target_store_registers (this, regnum);
>  

So, there's no place I can see where the regcache will reset its
m_register_status field to nullptr.  So my guess of what happens is that
when something wrong happens with the network connection inside
target_store_registers, the remote target closes, and somewhere in the
process, it destroys all regcaches bound to it (including the regcache
we are "in" right now).  This calls m_register_status' destructor, which
resets the internal pointer to nullptr.  When you reach the invalidator
code, the regcache has been deleted, so even accessing
this->m_register_status would be invalid, *this has been deleted.

If you run this with an ASan-enabled build, do you get a use-after-free
error?

Simon