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

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

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

No, that's fine.  I do with more people would post vN patches as follow
ups to their original messages, it would make tracking revisions so much
easier.

Maybe one day we'll move off the mailing list model.

Thanks,
Andrew