diff mbox

Avoid buffer overflow in value_x_unop

Message ID 20181128173804.10594-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Nov. 28, 2018, 5:38 p.m. UTC 
  Commit 6b1747cd1 ("invoke_xmethod & array_view") contains this change:

-  argvec = (struct value **) alloca (sizeof (struct value *) * 4);
+  value *argvec_storage[3];
+  gdb::array_view<value *> argvec = argvec_storage;

However, value_x_unop still does:

      argvec[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
      argvec[3] = 0;

This triggers an error with -fsanitize=address from userdef.exp:

ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdcf185068 at pc 0x000000e4f912 bp 0x7ffdcf184d80 sp 0x7ffdcf184d70
WRITE of size 8 at 0x7ffdcf185068 thread T0
    #0 0xe4f911 in value_x_unop(value*, exp_opcode, noside) ../../binutils-gdb/gdb/valarith.c:557
[...]

I think the two assignments to argvec[3] should just be removed, and
that this was intended in the earlier patch but just missed.

This passes userdef.exp with -fsanitize=address.

gdb/ChangeLog
2018-11-28  Tom Tromey  <tom@tromey.com>

	* valarith.c (value_x_unop): Don't set argvec[3].
---
 gdb/ChangeLog  | 4 ++++
 gdb/valarith.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Pedro Alves Nov. 29, 2018, 2:04 p.m. UTC | #1 
On 11/28/2018 05:38 PM, Tom Tromey wrote:
> Commit 6b1747cd1 ("invoke_xmethod & array_view") contains this change:
> 
> -  argvec = (struct value **) alloca (sizeof (struct value *) * 4);
> +  value *argvec_storage[3];
> +  gdb::array_view<value *> argvec = argvec_storage;
> 
> However, value_x_unop still does:
> 
>       argvec[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
>       argvec[3] = 0;
> 
> This triggers an error with -fsanitize=address from userdef.exp:
> 
> ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdcf185068 at pc 0x000000e4f912 bp 0x7ffdcf184d80 sp 0x7ffdcf184d70
> WRITE of size 8 at 0x7ffdcf185068 thread T0
>     #0 0xe4f911 in value_x_unop(value*, exp_opcode, noside) ../../binutils-gdb/gdb/valarith.c:557
> [...]
> 
> I think the two assignments to argvec[3] should just be removed, and
> that this was intended in the earlier patch but just missed.

Indeed it was.  Sorry about that.

> 
> This passes userdef.exp with -fsanitize=address.
> 
> gdb/ChangeLog
> 2018-11-28  Tom Tromey  <tom@tromey.com>
> 
> 	* valarith.c (value_x_unop): Don't set argvec[3].

OK.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/valarith.c b/gdb/valarith.c
index 3a59ada2d5..66cd9042d4 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -554,13 +554,11 @@  value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
     case UNOP_POSTINCREMENT:
       strcpy (ptr, "++");
       argvec[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
-      argvec[3] = 0;
       nargs ++;
       break;
     case UNOP_POSTDECREMENT:
       strcpy (ptr, "--");
       argvec[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
-      argvec[3] = 0;
       nargs ++;
       break;
     case UNOP_LOGICAL_NOT: