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

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Tom de Vries <tdevries@suse.de> writes:
>
>> PR gdb/33754 reports a heap-buffer-overflow here in args_complete_p:
>> ...
>>   while (*input != '\0')
>> ...
>>
>> Fix this by introducing a lambda function at that safely handles all char
>> array accesses.
>
> Sorry to be a bore, but after reading this commit, and the bug report,
> it's still not obvious to me where the overflow actually occurs.
>
> I totally accept that this code is broken, but as I introduced this bug,
> I wanted to learn from this mistake, but this commit doesn't really
> explain what mistake is being fixed.
>
> Do you think you could explain what's actually going wrong here?

Literally after hitting send, it occurred to me, is the problem maybe
these two lines:

  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
    ++input;

And the other one in the 'else' block?  I think if *(input + 1) is '\0',
then the strchr call will return non-nullptr, which wasn't the desired
behaviour, and could result in stepping outside the string.

If this is the case then I think the correct fix would be checking if
the character at 'input + 1' is NULL or not, see the possible patch
below (there's no commit message or anything for it yet).

Thanks,
Andrew

---

diff --git i/gdb/infcmd.c w/gdb/infcmd.c
index 875bbe1ee69..7dd3392c96a 100644
--- i/gdb/infcmd.c
+++ w/gdb/infcmd.c
@@ -148,7 +148,9 @@ args_complete_p (const std::string &args)
 	     and we don't skip the entire '\\' then we'll only skip the
 	     first '\', in which case we might see the second '\' as a '\"'
 	     sequence, which would be wrong.  */
-	  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
+	  if (*input == '\\'
+	      && *(input + 1) != '\0'
+	      && strchr ("\"\\", *(input + 1)) != nullptr)
 	    ++input;
 	  /* Otherwise, just look for the closing double quote.  */
 	  else if (*input == '"')
@@ -162,7 +164,9 @@ args_complete_p (const std::string &args)
 	     a quoted argument.  The '\\' we need to skip so we don't just
 	     skip the first '\' and then incorrectly consider the second
 	     '\' are part of a '\"' or '\'' sequence.  */
-	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
+	  if (*input == '\\'
+	      && *(input + 1) != '\0'
+	      && strchr ("\"\\'", *(input + 1)) != nullptr)
 	    ++input;
 	  /* Otherwise, check for the start of a single or double quoted
 	     argument.  Single quotes have no special meaning on Windows