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

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

On 1/7/26 11:46 AM, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> 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 == " ".
> 
> Thanks for breaking it down for me.  I don't really like the original
> lambda function approach that was proposed, I'd prefer to just see the
> correct checks added to the loop.  More inline below...
> 
>>
>> 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;
> 
> I think I prefer this to Tom's proposed 'for' loop, but I don't feel
> super strongly each way.
> 
>>
>>          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;
> 
> I'd replace this with 'gdb_assert (*input != '\0');', and then use
> something like the extra check I proposed next to the strchr calls.  Or
> maybe we should add a new helper function in gdbsupport/ like:
> 
>    static char *
>    strchr_not_null (char *s, int c)
>    {
>      if (c == '\0')
>        return nullptr;
>    
>      return strchr (s, c);
>    }
>    
>    static const char *
>    strchr_not_null (const char *s, int c)
>    {
>      return strchr_not_null (const_cast<char *> (s), c);
>    }
> 
> which wraps the null check.  Either would be fine with me.
> 
> I also liked the selftests you added, I extended them to:
> 
>    static void
>    check_str (const std::string &str, bool complete_p)
>    {
>      const char *end;
>    
>      SELF_CHECK (args_complete_p (str, &end) == complete_p);
>      SELF_CHECK (end == str.data () + str.size ());
>    }
>    
>    static void
>    infcmd_args_complete_p_tests (void)
>    {
>      check_str (" ", true);
>      check_str ("\\", true);
>      check_str ("\"\\", false);
>    }
> 
> which covers all the bugs that are being fixed here.
> 

Hi Andrew,

thanks for the comments.

But by now, a v2 was submitted, approved and committed.

So perhaps you want to submit a refactoring patch addressing some of 
your insights here.  Otherwise, I can take it further.  Let me know what 
you prefer.

Thanks,
- Tom