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

Andrew Burgess
2026-01-07T10:47:25Z

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.

Thanks,
Andrew