Don't pass NULL to memcpy in gdb

Message ID 20200320150154.29206-1-tromey@adacore.com
State New
Headers show
Series
  • Don't pass NULL to memcpy in gdb
Related show

Commit Message

Tom Tromey March 20, 2020, 3:01 p.m.
I compiled gdb with -fsanitize=undefined and ran the test suite.

A couple of reports came from passing NULL to memcpy, e.g.:

[...]btrace-common.cc:176:13: runtime error: null pointer passed as argument 2, which is declared to never be null

While it would be better to fix this in the standard, in the meantime
it seems easy to avoid this error.

gdb/ChangeLog
2020-03-20  Tom Tromey  <tromey@adacore.com>

	* dwarf2/abbrev.c (abbrev_table::read): Conditionally call
	memcpy.

gdbsupport/ChangeLog
2020-03-20  Tom Tromey  <tromey@adacore.com>

	* btrace-common.cc (btrace_data_append): Conditionally call
	memcpy.
---
 gdb/ChangeLog               | 5 +++++
 gdb/dwarf2/abbrev.c         | 5 +++--
 gdbsupport/ChangeLog        | 5 +++++
 gdbsupport/btrace-common.cc | 3 ++-
 4 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.21.1

Comments

Simon Marchi March 21, 2020, 2:51 a.m. | #1
On 2020-03-20 11:01 a.m., Tom Tromey wrote:
> I compiled gdb with -fsanitize=undefined and ran the test suite.

> 

> A couple of reports came from passing NULL to memcpy, e.g.:

> 

> [...]btrace-common.cc:176:13: runtime error: null pointer passed as argument 2, which is declared to never be null

> 

> While it would be better to fix this in the standard, in the meantime

> it seems easy to avoid this error.


I hope you're joking here :).  But otherwise, this LGTM.

Simon
Tom Tromey March 22, 2020, 7:04 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> While it would be better to fix this in the standard, in the meantime

>> it seems easy to avoid this error.


Simon> I hope you're joking here :).  But otherwise, this LGTM.

I actually do think it would be better for the standard to change to say
that memcpy should accept NULL pointers provided that the length is 0.
Probably most implementations already work this way and it seems
harmless to allow.  It also seems relatively unlikely to change though.

Tom

Patch

diff --git a/gdb/dwarf2/abbrev.c b/gdb/dwarf2/abbrev.c
index 59ff138b33d..b85018060fa 100644
--- a/gdb/dwarf2/abbrev.c
+++ b/gdb/dwarf2/abbrev.c
@@ -168,8 +168,9 @@  abbrev_table::read (struct objfile *objfile,
       cur_abbrev->attrs =
 	XOBNEWVEC (&abbrev_table->m_abbrev_obstack, struct attr_abbrev,
 		   cur_abbrev->num_attrs);
-      memcpy (cur_abbrev->attrs, cur_attrs.data (),
-	      cur_abbrev->num_attrs * sizeof (struct attr_abbrev));
+      if (!cur_attrs.empty ())
+	memcpy (cur_abbrev->attrs, cur_attrs.data (),
+		cur_abbrev->num_attrs * sizeof (struct attr_abbrev));
 
       abbrev_table->add_abbrev (abbrev_number, cur_abbrev);
 
diff --git a/gdbsupport/btrace-common.cc b/gdbsupport/btrace-common.cc
index 7d4f6424c82..e8b24db7d53 100644
--- a/gdbsupport/btrace-common.cc
+++ b/gdbsupport/btrace-common.cc
@@ -173,7 +173,8 @@  btrace_data_append (struct btrace_data *dst,
 	    size = src->variant.pt.size + dst->variant.pt.size;
 	    data = (gdb_byte *) xmalloc (size);
 
-	    memcpy (data, dst->variant.pt.data, dst->variant.pt.size);
+	    if (dst->variant.pt.size > 0)
+	      memcpy (data, dst->variant.pt.data, dst->variant.pt.size);
 	    memcpy (data + dst->variant.pt.size, src->variant.pt.data,
 		    src->variant.pt.size);