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