[PATCH][gdb] Fix heap-buffer-overflow in completion_tracker::build_completion_result

[PATCH][gdb] Fix heap-buffer-overflow in completion_tracker::build_completion_result

[Tom de Vries]

Hi,

When building gdb with address sanitizer and running test-case
gdb.base/completion.exp, we run into:
...
==5743==ERROR: AddressSanitizer: heap-buffer-overflow on address \
  0x60200025c02f at pc 0x000000cd9d64 bp 0x7fff3297da30 sp 0x7fff3297da28
READ of size 1 at 0x60200025c02f thread T0
    #0 0xcd9d63 in completion_tracker::build_completion_result(char const*, \
                     int, int) gdb/completer.c:2258
  ...
0x60200025c02f is located 1 bytes to the left of 1-byte region \
  [0x60200025c030,0x60200025c031)
...

The problem is in this code in completion_tracker::build_completion_result:
...
      bool completion_suppress_append
        = (suppress_append_ws ()
           || match_list[0][strlen (match_list[0]) - 1] == ' ');
...
If strlen (match_list[0]) == 0, then we access match_list[0][-1].

Fix this by testing if the memory access is in bounds before doing the memory
access.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb] Fix heap-buffer-overflow in completion_tracker::build_completion_result

gdb/ChangeLog:

2020-12-04  Tom de Vries  <tdevries@suse.de>

	PR gdb/27003
	* completer.c (completion_tracker::build_completion_result): Don't access
	match_list[0][-1].

---
 gdb/completer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 262c8556bf..83b46a0e4d 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2253,9 +2253,11 @@ completion_tracker::build_completion_result (const char *text,
       /* If the tracker wants to, or we already have a space at the
 	 end of the match, tell readline to skip appending
 	 another.  */
+      char *match = match_list[0];
       bool completion_suppress_append
 	= (suppress_append_ws ()
-	   || match_list[0][strlen (match_list[0]) - 1] == ' ');
+	   || (match[0] != '\0'
+	       && match[strlen (match) - 1] == ' '));
 
       return completion_result (match_list, 1, completion_suppress_append);
     }

Re: [PATCH][gdb] Fix heap-buffer-overflow in completion_tracker::build_completion_result

[Pedro Alves]

On 12/4/20 5:39 PM, Tom de Vries wrote:
> Hi,
> 
> When building gdb with address sanitizer and running test-case
> gdb.base/completion.exp, we run into:
> ...
> ==5743==ERROR: AddressSanitizer: heap-buffer-overflow on address \
>   0x60200025c02f at pc 0x000000cd9d64 bp 0x7fff3297da30 sp 0x7fff3297da28
> READ of size 1 at 0x60200025c02f thread T0
>     #0 0xcd9d63 in completion_tracker::build_completion_result(char const*, \
>                      int, int) gdb/completer.c:2258
>   ...
> 0x60200025c02f is located 1 bytes to the left of 1-byte region \
>   [0x60200025c030,0x60200025c031)
> ...
> 

It would be useful to show which test triggered this.  I checked, and it's
the new:

 (gdb) p/d[TAB]

test.

> The problem is in this code in completion_tracker::build_completion_result:
> ...
>       bool completion_suppress_append
>         = (suppress_append_ws ()
>            || match_list[0][strlen (match_list[0]) - 1] == ' ');
> ...
> If strlen (match_list[0]) == 0, then we access match_list[0][-1].
> 
> Fix this by testing if the memory access is in bounds before doing the memory
> access.
> 
> Tested on x86_64-linux.
> 
> Any comments?

My first question is -- why do we end up here in the first place?
Why is m_lowest_common_denominator_unique set with the lcd empty?

The answer is that the new "/FMT" completer pushes an empty
completion match:

	  if (*text == '\0')
	    {
	      /* We're at the end of the input string.  The user has typed
		 '/FMT' and asked for a completion.  Push an empty
		 completion string, this will cause readline to insert a
		 space so the user now has '/FMT '.  */
	      in_fmt = true;
	      tracker.add_completion (make_unique_xstrdup (text));
	    }

This could also be fixed by adding a non-empty completion and not advancing
the completion word point, like:

 -             tracker.add_completion (make_unique_xstrdup (text));
 +             tracker.add_completion (make_unique_xstrdup (*args));
 +             return true;

but I'm not thinking of a real problem the empty completion match
causes, so I think it's OK to properly support it.

So the patch is OK.  But please mention the "p/d[TAB]" scenario in
the commit log.