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

[Tom de Vries <tdevries@suse.de>]

On 1/5/26 8:57 PM, Andrew Burgess wrote:
> 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?
> 

Hi Andrew,

agreed, it's not spelled out, sorry about that.

So, the heap-buffer-overflow happens with:
...
(gdb) p args
$1 = "\"first arg\" \"\" \"third-arg\" \"'\" \"\\\"\" \" \" \"\" "
...
and it's the fact that we don't check for '\0' after skip_spaces that is 
the problem.  I think it should be possible to reproduce the problem 
with args == " ".

So a minimal fix for this problem is:
...
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 1a7daf1461b..fdcd4e4ba96 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -131,6 +131,8 @@ args_complete_p (const std::string &args)
    while (*input != '\0')
      {
        input = skip_spaces (input);
+      if (*input == '\0')
+	break;

        if (squote)
  	{
...

But the strchr problem is also there, so this:
...
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 1a7daf1461b..4bcd523f79b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -177,6 +177,8 @@ args_complete_p (const std::string &args)
  	    dquote = true;
  	}

+      if (*input == '\0')
+	break;
        ++input;
      }

...
would catch both, I think.  Not that I'm suggesting this fix.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
> 
> 
>>
>> Also:
>> - factor out char array accesses using new variables c and next_c, and
>> - check for end-of-string after skip_spaces.
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754
>> ---
>>   gdb/infcmd.c | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 875bbe1ee69..ceacfd05683 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -126,17 +126,27 @@ static bool
>>   args_complete_p (const std::string &args)
>>   {
>>     const char *input = args.c_str ();
>> +  const char *end = input + args.length ();
>>     bool squote = false, dquote = false;
>>   
>> -  while (*input != '\0')
>> +  auto at = [&] (const char *s)
>> +    {
>> +      return s > end ? '\0' : *s;
>> +    };
>> +
>> +  while (at (input) != '\0')
>>       {
>>         input = skip_spaces (input);
>> +      char c = at (input);
>> +      if (c == '\0')
>> +	break;
>> +      char next_c = at (input + 1);
>>   
>>         if (squote)
>>   	{
>>   	  /* Inside a single quoted argument, look for the closing single
>>   	     quote.  */
>> -	  if (*input == '\'')
>> +	  if (c == '\'')
>>   	    squote = false;
>>   	}
>>         else if (dquote)
>> @@ -148,10 +158,10 @@ 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 (c == '\\' && strchr ("\"\\", next_c) != nullptr)
>>   	    ++input;
>>   	  /* Otherwise, just look for the closing double quote.  */
>> -	  else if (*input == '"')
>> +	  else if (c == '"')
>>   	    dquote = false;
>>   	}
>>         else
>> @@ -162,7 +172,7 @@ 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 (c == '\\' && strchr ("\"\\'", next_c) != nullptr)
>>   	    ++input;
>>   	  /* Otherwise, check for the start of a single or double quoted
>>   	     argument.  Single quotes have no special meaning on Windows
>> @@ -170,10 +180,10 @@ args_complete_p (const std::string &args)
>>   	     host to determine what is, or isn't a special character, when
>>   	     really, this is a function of the target.  */
>>   #ifndef _WIN32
>> -	  else if (*input == '\'')
>> +	  else if (c == '\'')
>>   	    squote = true;
>>   #endif
>> -	  else if (*input == '"')
>> +	  else if (c == '"')
>>   	    dquote = true;
>>   	}
>>   
>>
>> base-commit: 0a153c58a0ab68c6fa349d2ad0bf6a42e043ab23
>> -- 
>> 2.51.0
>