[PATCH v2] Fix dereference of possible nullptr in -O3

[PATCH v2] Fix dereference of possible nullptr in -O3

[Simon Farre]

v2. Formatting.

v1: Building with full optimizations (-O3) complains that this might
be a nullptr. If this is never null, perhaps the parameter
should be changed to a reference, but that would change the interface.
---
 gdb/regcache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index a5e83da1ca4..20a4ddcd52a 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1173,7 +1173,8 @@ regcache::transfer_regset_register (struct regcache *out_regcache, int regnum,
   else
     {
       /* Invalidate the register.  */
-      out_regcache->raw_supply (regnum, nullptr);
+      if (out_regcache != nullptr)
+	out_regcache->raw_supply (regnum, nullptr);
     }
 }
 

base-commit: 16089f320a9226e7cdb73e9fb4266d9e450085b2
-- 
2.34.1

Re: [PATCH v2] Fix dereference of possible nullptr in -O3

[Pedro Alves]

On 2022-05-10 18:22, Simon Farre via Gdb-patches wrote:
> v2. Formatting.
> 
> v1: Building with full optimizations (-O3) complains that this might
> be a nullptr. If this is never null, perhaps the parameter
> should be changed to a reference, but that would change the interface.

Needs more detail.  What is the compiler in question?  What does the warning look like?

Regardless, as people mentioned in v1, this isn't the right fix.  At best, it should probably
be an:

  gdb_assert (out_regcache != nullptr);

just because the supposed-null-deference.  That should quiet such a warning.

I only see two calls to transfer_regset (which forwards arguments to transfer_regset_register):

#1 - transfer_regset (regset, this, regnum, (const gdb_byte *) buf, nullptr, size);

#2 - transfer_regset (regset, nullptr, regnum, nullptr, (gdb_byte *) buf, size);

The case in question is when all of out_regcache, in_buf and out_buf are NULL, which
could only happen if the "buf" argument in the second transfer_regset above
was NULL.  I don't think that is supposed to ever happen.

The right assert is probably this at the top of the function:

  gdb_assert ((out_regcache != nullptr) != (out_buf != nullptr));

... meaning, you must pass either a non-NULL out_regcache or a non-NULL out_buf,
but never both NULL, and never both non-NULL.  This should quiet the compiler
warning too, hopefully.

Re: [PATCH v2] Fix dereference of possible nullptr in -O3

[Simon Farre]

> Needs more detail.  What is the compiler in question?  What does the
warning look like?

The compiler in question is GCC (11.2.0-19ubuntu1) and the "error" is due
to inlining of virtual functions. Output from the compiler can be found at
the bottom.
-O2 do not perform as aggressive optimizations as -O3 and thus do not "hit"
this error.
I tried to compile it with clang-13, but it instead generates other errors,
so I'm not able to test -O3 using clang unfortunately.

> I only see two calls to transfer_regset (which forwards arguments to
transfer_regset_register):

> #1 - transfer_regset (regset, this, regnum, (const gdb_byte *) buf,
nullptr, size);
> #2 - transfer_regset (regset, nullptr, regnum, nullptr, (gdb_byte *) buf,
size);
Yes, there's only two calls to transfer_regset, I was looking further up
the call chain, but you're right those are the only two places
that directly call that method, my mistake.

> The right assert is probably this at the top of the function:
>  gdb_assert ((out_regcache != nullptr) != (out_buf != nullptr));

> ... meaning, you must pass either a non-NULL out_regcache or a non-NULL
out_buf,
> but never both NULL, and never both non-NULL.  This should quiet the
compiler
> warning too, hopefully.

If you think that adding an assert here is the right approach, I'll add it
in v.3.

Thanks!
Simon

In member function ‘virtual void reg_buffer::raw_supply(int, const void*)’,
    inlined from ‘void regcache::transfer_regset_register(regcache*, int,
const gdb_byte*, gdb_byte*, int, int) const’ at
.../binutils-gdb/gdb/regcache.c:1176:32,
    inlined from ‘void regcache::transfer_regset(const regset*, regcache*,
int, const gdb_byte*, gdb_byte*, size_t) const’ at
.../binutils-gdb/gdb/regcache.c:1212:31:
/.../binutils-gdb/gdb/regcache.c:1053:17: error: ‘this’ pointer is null
[-Werror=nonnull]
 1053 |   assert_regnum (regnum);
      |   ~~~~~~~~~~~~~~^~~~~~~~
.../binutils-gdb/gdb/regcache.c: In function ‘void
regcache::transfer_regset(const regset*, regcache*, int, const gdb_byte*,
gdb_byte*, size_t) const’:
.../binutils-gdb/gdb/regcache.c:311:1: note: in a call to non-static member
function ‘void reg_buffer::assert_regnum(int) const’
  311 | reg_buffer::assert_regnum (int regnum) const
      | ^~~~~~~~~~
In member function ‘void regcache::transfer_regset_register(regcache*, int,
const gdb_byte*, gdb_byte*, int, int) const’,
    inlined from ‘void regcache::transfer_regset(const regset*, regcache*,
int, const gdb_byte*, gdb_byte*, size_t) const’ at
.../binutils-gdb/gdb/regcache.c:1212:31:
.../binutils-gdb/gdb/regcache.c:1176:32: error: ‘this’ pointer is null
[-Werror=nonnull]
 1176 |       out_regcache->raw_supply (regnum, nullptr);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In member function ‘void regcache::transfer_regset_register(regcache*, int,
const gdb_byte*, gdb_byte*, int, int) const’,
    inlined from ‘void regcache::transfer_regset(const regset*, regcache*,
int, const gdb_byte*, gdb_byte*, size_t) const’ at
.../binutils-gdb/gdb/regcache.c:1222:29:
.../binutils-gdb/gdb/regcache.c:1176:32: error: ‘this’ pointer is null
[-Werror=nonnull]
 1176 |       out_regcache->raw_supply (regnum, nullptr);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~