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

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

On 1/6/26 9:47 AM, Tom de Vries wrote:
> 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.
> 

I've submitted a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2026-January/223715.html ) 
handling both problems explicitly.

Thanks,
- Tom

> 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
>>
>