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

Tom de Vries
2026-01-07T11:21:50Z

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