[patch] Fix -fsanitize=address on unreadable inferior strings

[patch] Fix -fsanitize=address on unreadable inferior strings

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

Hi,

it is not a real memory corruption/crash but it breaks with ASAN, making the
use of ASAN for regular GDB runs difficult.


echo 'void f(char *s){}main(){f((char *)1);}'|gcc -g -x c -;../gdb ./a.out -ex 'b f' -ex r
====ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000aaccf at pc 0x96eea7 bp 0x7fff75bdbc90 sp 0x7fff75bdbc80
READ of size 1 at 0x6020000aaccf thread T0
    #0 0x96eea6 in extract_unsigned_integer .../gdb/findvar.c:108
    #1 0x9df02b in val_print_string .../gdb/valprint.c:2513
[...]
0x6020000aaccf is located 1 bytes to the left of 8-byte region [0x6020000aacd0,0x6020000aacd8)
allocated by thread T0 here:
    #0 0x7f45fad26b97 in malloc (/lib64/libasan.so.1+0x57b97)
    #1 0xdb3409 in xmalloc common/common-utils.c:45
    #2 0x9d8cf9 in read_string .../gdb/valprint.c:1845
    #3 0x9defca in val_print_string .../gdb/valprint.c:2502
[..]
====ABORTING


No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.

I do not think it can cause any behavior change.


Thanks,
Jan

[-- Attachment #2: asan-string.patch --]
[-- Type: text/plain, Size: 1555 bytes --]

echo 'void f(char *s){}main(){f((char *)1);}'|gcc -g -x c -;../gdb ./a.out -ex 'b f' -ex r
====ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000aaccf at pc 0x96eea7 bp 0x7fff75bdbc90 sp 0x7fff75bdbc80
READ of size 1 at 0x6020000aaccf thread T0
    #0 0x96eea6 in extract_unsigned_integer .../gdb/findvar.c:108
    #1 0x9df02b in val_print_string .../gdb/valprint.c:2513
[...]
0x6020000aaccf is located 1 bytes to the left of 8-byte region [0x6020000aacd0,0x6020000aacd8)
allocated by thread T0 here:
    #0 0x7f45fad26b97 in malloc (/lib64/libasan.so.1+0x57b97)
    #1 0xdb3409 in xmalloc common/common-utils.c:45
    #2 0x9d8cf9 in read_string .../gdb/valprint.c:1845
    #3 0x9defca in val_print_string .../gdb/valprint.c:2502
[..]
====ABORTING

gdb/
2014-08-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix -fsanitize=address on unreadable inferior strings.
	* valprint.c (val_print_string): Fix access before BUFFER.

diff --git a/gdb/valprint.c b/gdb/valprint.c
index d3ab267..a87d67c 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2510,8 +2510,10 @@ val_print_string (struct type *elttype, const char *encoding,
      LEN is -1.  */
 
   /* Determine found_nul by looking at the last character read.  */
-  found_nul = extract_unsigned_integer (buffer + bytes_read - width, width,
-					byte_order) == 0;
+  found_nul = 0;
+  if (bytes_read >= width)
+    found_nul = extract_unsigned_integer (buffer + bytes_read - width, width,
+					  byte_order) == 0;
   if (len == -1 && !found_nul)
     {
       gdb_byte *peekbuf;

Re: [patch] Fix -fsanitize=address on unreadable inferior strings

[Joel Brobecker]

> gdb/
> 2014-08-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix -fsanitize=address on unreadable inferior strings.
> 	* valprint.c (val_print_string): Fix access before BUFFER.

LGTM. I actually don't see why this wouldn't be a possible bug.
Couldn't LEN be 0, in which case BYTES_READ would be zero, making
WIDTH > BYTES_READ?

> 
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index d3ab267..a87d67c 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -2510,8 +2510,10 @@ val_print_string (struct type *elttype, const char *encoding,
>       LEN is -1.  */
>  
>    /* Determine found_nul by looking at the last character read.  */
> -  found_nul = extract_unsigned_integer (buffer + bytes_read - width, width,
> -					byte_order) == 0;
> +  found_nul = 0;
> +  if (bytes_read >= width)
> +    found_nul = extract_unsigned_integer (buffer + bytes_read - width, width,
> +					  byte_order) == 0;
>    if (len == -1 && !found_nul)
>      {
>        gdb_byte *peekbuf;


-- 
Joel

Re: [patch] Fix -fsanitize=address on unreadable inferior strings

[Jan Kratochvil]

On Tue, 19 Aug 2014 08:54:22 +0200, Joel Brobecker wrote:
> > gdb/
> > 2014-08-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix -fsanitize=address on unreadable inferior strings.
> > 	* valprint.c (val_print_string): Fix access before BUFFER.
> 
> LGTM. I actually don't see why this wouldn't be a possible bug.
> Couldn't LEN be 0, in which case BYTES_READ would be zero, making
> WIDTH > BYTES_READ?

If LEN == 0 then the whole GDB code ignores FOUND_NUL, therefore it is
irrelevant for this patch.  Besides that if LEN == 0 then ERRCODE == 0,
therefore FORCE_ELLIPSIS == 0.

A sort of bug is that if the start of string memory is unreadable GDB randomly
may attempt to read the same byte/character second time (failing again).
So it is sort of a performance bug.

(Although such read of the same memory should be cached which I did not check
now if it really is.)

I will therefore check it in.


Jan

[commit] [patch] Fix -fsanitize=address on unreadable inferior strings

[Jan Kratochvil]

On Tue, 19 Aug 2014 08:54:22 +0200, Joel Brobecker wrote:
> > gdb/
> > 2014-08-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Fix -fsanitize=address on unreadable inferior strings.
> > 	* valprint.c (val_print_string): Fix access before BUFFER.
> 
> LGTM.

Checked in:
	6694c4110a37bc951d01132d6e56445d57350627


Jan