Undefined behavior reported in copy_bitwise

Message ID CAMCMYR5=V6WBN-AZw4R8EAVxkRt4T62G0JCARAaCc77PiwF7LQ@mail.gmail.com
State New
Headers show
Series
  • Undefined behavior reported in copy_bitwise
Related show

Commit Message

Pedro Alves via Gdb-patches April 9, 2020, 11:50 p.m.
gdb version 9.1, built with clang 8.0.0 on Ubuntu 18.04 (x86_64);
--enable-ubsan (for clang's undefined behavior sanitizer)

Executing command; `maint selftest copy_bitwise` bombs in runtime error:
../../gdb/utils.c:3432:28: runtime error: left shift of negative value -1

Closer look reveals the offending shift: `(~0 << nbits)`, apparently 0
is treated as signed int, resulting in negative complement. Explicitly
stating it unsigned 0U  fixes it and the `copy_bitwise` test passes
ok.

patch -p1:
````````

Comments

Simon Marchi April 10, 2020, 2:59 p.m. | #1
On 2020-04-09 7:50 p.m., krokus via Gdb-patches wrote:
> gdb version 9.1, built with clang 8.0.0 on Ubuntu 18.04 (x86_64);

> --enable-ubsan (for clang's undefined behavior sanitizer)

> 

> Executing command; `maint selftest copy_bitwise` bombs in runtime error:

> ../../gdb/utils.c:3432:28: runtime error: left shift of negative value -1

> 

> Closer look reveals the offending shift: `(~0 << nbits)`, apparently 0

> is treated as signed int, resulting in negative complement. Explicitly

> stating it unsigned 0U  fixes it and the `copy_bitwise` test passes

> ok.

> 

> patch -p1:

> ````````

> --- gdb/utils.c    2020-04-09 18:41:03.339065535 -0500

> +++ gdb/utils.c    2020-04-09 18:41:24.427064851 -0500

> @@ -3429,7 +3429,7 @@

>      buf |= *source << avail;

> 

>        buf &= (1 << nbits) - 1;

> -      *dest = (*dest & (~0 << nbits)) | buf;

> +      *dest = (*dest & (~0U << nbits)) | buf;

>      }

>  }

> ````````

> 


Thanks.  I'll try to reproduce it, but at first sight it seems fine.

Could you please provide a ChangeLog entry for this, with the name and
email address you'd like to have in there?  See the gdb/ChangeLog file
for examples.

Simon
Pedro Alves via Gdb-patches April 10, 2020, 8:56 p.m. | #2
Simon,

> Could you please provide a ChangeLog entry for this, with the name and

> email address you'd like to have in there?


Will do.
Do I need to update the ChangeLog myself or you could do this with my entry?
I'm not set up for repo commit access.
Simon Marchi April 10, 2020, 9:24 p.m. | #3
On 2020-04-10 4:56 p.m., krokus via Gdb-patches wrote:
> Simon,

> 

>> Could you please provide a ChangeLog entry for this, with the name and

>> email address you'd like to have in there?

> 

> Will do.

> Do I need to update the ChangeLog myself or you could do this with my entry?

> I'm not set up for repo commit access.

> 


Just provide it here in the email body, I'll take care of formatting and pushing
the patch.

Thanks,

Simon
Simon Marchi April 11, 2020, 1:08 a.m. | #4
On 2020-04-10 5:24 p.m., Simon Marchi wrote:
> On 2020-04-10 4:56 p.m., krokus via Gdb-patches wrote:

>> Simon,

>>

>>> Could you please provide a ChangeLog entry for this, with the name and

>>> email address you'd like to have in there?

>>

>> Will do.

>> Do I need to update the ChangeLog myself or you could do this with my entry?

>> I'm not set up for repo commit access.

>>

> 

> Just provide it here in the email body, I'll take care of formatting and pushing

> the patch.


Thanks, I've pushed the patch as below.  I've modified the ChangeLog entry a bit
to more clearly state what changed in the affected function, but that's it.


From cf83625da29d1239e97f1eb4d145f347cb741889 Mon Sep 17 00:00:00 2001
From: Artur Shepilko <nomadbyte@gmail.com>

Date: Fri, 10 Apr 2020 10:56:43 -0400
Subject: [PATCH] gdb: fix undefined behavior reported in copy_bitwise

gdb version 9.1, built with clang 8.0.0 on Ubuntu 18.04 (x86_64);
--enable-ubsan (for clang's undefined behavior sanitizer)

Executing command; `maint selftest copy_bitwise` bombs in runtime error:
../../gdb/utils.c:3432:28: runtime error: left shift of negative value -1

Closer look reveals the offending shift: `(~0 << nbits)`, apparently 0
is treated as signed int, resulting in negative complement. Explicitly
stating it unsigned 0U  fixes it and the `copy_bitwise` test passes
ok.
---
 gdb/ChangeLog | 5 +++++
 gdb/utils.c   | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 661a41467bbe..81102ee569bc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-04-10  Artur Shepilko  <nomadbyte@gmail.com>
+
+	* utils.c (copy_bitwise): Use unsigned 0 constant as operand of
+	bit shift.
+
 2020-04-10  Tom Tromey  <tromey@adacore.com>

 	* symfile.c (symbol_file_add_separate): Preserve OBJF_MAINLINE.
diff --git a/gdb/utils.c b/gdb/utils.c
index bda6bbf5b0e7..f5b20331b1e2 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3433,7 +3433,7 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
 	buf |= *source << avail;

       buf &= (1 << nbits) - 1;
-      *dest = (*dest & (~0 << nbits)) | buf;
+      *dest = (*dest & (~0U << nbits)) | buf;
     }
 }

-- 
2.26.0

Patch

--- gdb/utils.c    2020-04-09 18:41:03.339065535 -0500
+++ gdb/utils.c    2020-04-09 18:41:24.427064851 -0500
@@ -3429,7 +3429,7 @@ 
     buf |= *source << avail;

       buf &= (1 << nbits) - 1;
-      *dest = (*dest & (~0 << nbits)) | buf;
+      *dest = (*dest & (~0U << nbits)) | buf;
     }
 }
````````