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

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

[Tom de Vries]

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.

Also:
- factor out char array accesses using new variables c and next_c, and
- check for end-of-string after skip_spaces.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754
---
 gdb/infcmd.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 875bbe1ee69..ceacfd05683 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -126,17 +126,27 @@ static bool
 args_complete_p (const std::string &args)
 {
   const char *input = args.c_str ();
+  const char *end = input + args.length ();
   bool squote = false, dquote = false;
 
-  while (*input != '\0')
+  auto at = [&] (const char *s)
+    {
+      return s > end ? '\0' : *s;
+    };
+
+  while (at (input) != '\0')
     {
       input = skip_spaces (input);
+      char c = at (input);
+      if (c == '\0')
+	break;
+      char next_c = at (input + 1);
 
       if (squote)
 	{
 	  /* Inside a single quoted argument, look for the closing single
 	     quote.  */
-	  if (*input == '\'')
+	  if (c == '\'')
 	    squote = false;
 	}
       else if (dquote)
@@ -148,10 +158,10 @@ args_complete_p (const std::string &args)
 	     and we don't skip the entire '\\' then we'll only skip the
 	     first '\', in which case we might see the second '\' as a '\"'
 	     sequence, which would be wrong.  */
-	  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
+	  if (c == '\\' && strchr ("\"\\", next_c) != nullptr)
 	    ++input;
 	  /* Otherwise, just look for the closing double quote.  */
-	  else if (*input == '"')
+	  else if (c == '"')
 	    dquote = false;
 	}
       else
@@ -162,7 +172,7 @@ args_complete_p (const std::string &args)
 	     a quoted argument.  The '\\' we need to skip so we don't just
 	     skip the first '\' and then incorrectly consider the second
 	     '\' are part of a '\"' or '\'' sequence.  */
-	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
+	  if (c == '\\' && strchr ("\"\\'", next_c) != nullptr)
 	    ++input;
 	  /* Otherwise, check for the start of a single or double quoted
 	     argument.  Single quotes have no special meaning on Windows
@@ -170,10 +180,10 @@ args_complete_p (const std::string &args)
 	     host to determine what is, or isn't a special character, when
 	     really, this is a function of the target.  */
 #ifndef _WIN32
-	  else if (*input == '\'')
+	  else if (c == '\'')
 	    squote = true;
 #endif
-	  else if (*input == '"')
+	  else if (c == '"')
 	    dquote = true;
 	}
 

base-commit: 0a153c58a0ab68c6fa349d2ad0bf6a42e043ab23
-- 
2.51.0

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

[Tom Tromey]

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

Tom> +  auto at = [&] (const char *s)
Tom> +    {
Tom> +      return s > end ? '\0' : *s;
Tom> +    };

I think it would better to avoid stepping off the end at the points
where the pointer is incremented.

Tom> +  while (at (input) != '\0')

Like this could be

   for (input = skip_spaces (input); *input != '\0'; input = skip_spaces (input))

and then there's like one or two spots to check in the loop.

Tom

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

[Andrew Burgess]

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?

Thanks,
Andrew




>
> Also:
> - factor out char array accesses using new variables c and next_c, and
> - check for end-of-string after skip_spaces.
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754
> ---
>  gdb/infcmd.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 875bbe1ee69..ceacfd05683 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -126,17 +126,27 @@ static bool
>  args_complete_p (const std::string &args)
>  {
>    const char *input = args.c_str ();
> +  const char *end = input + args.length ();
>    bool squote = false, dquote = false;
>  
> -  while (*input != '\0')
> +  auto at = [&] (const char *s)
> +    {
> +      return s > end ? '\0' : *s;
> +    };
> +
> +  while (at (input) != '\0')
>      {
>        input = skip_spaces (input);
> +      char c = at (input);
> +      if (c == '\0')
> +	break;
> +      char next_c = at (input + 1);
>  
>        if (squote)
>  	{
>  	  /* Inside a single quoted argument, look for the closing single
>  	     quote.  */
> -	  if (*input == '\'')
> +	  if (c == '\'')
>  	    squote = false;
>  	}
>        else if (dquote)
> @@ -148,10 +158,10 @@ args_complete_p (const std::string &args)
>  	     and we don't skip the entire '\\' then we'll only skip the
>  	     first '\', in which case we might see the second '\' as a '\"'
>  	     sequence, which would be wrong.  */
> -	  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
> +	  if (c == '\\' && strchr ("\"\\", next_c) != nullptr)
>  	    ++input;
>  	  /* Otherwise, just look for the closing double quote.  */
> -	  else if (*input == '"')
> +	  else if (c == '"')
>  	    dquote = false;
>  	}
>        else
> @@ -162,7 +172,7 @@ args_complete_p (const std::string &args)
>  	     a quoted argument.  The '\\' we need to skip so we don't just
>  	     skip the first '\' and then incorrectly consider the second
>  	     '\' are part of a '\"' or '\'' sequence.  */
> -	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
> +	  if (c == '\\' && strchr ("\"\\'", next_c) != nullptr)
>  	    ++input;
>  	  /* Otherwise, check for the start of a single or double quoted
>  	     argument.  Single quotes have no special meaning on Windows
> @@ -170,10 +180,10 @@ args_complete_p (const std::string &args)
>  	     host to determine what is, or isn't a special character, when
>  	     really, this is a function of the target.  */
>  #ifndef _WIN32
> -	  else if (*input == '\'')
> +	  else if (c == '\'')
>  	    squote = true;
>  #endif
> -	  else if (*input == '"')
> +	  else if (c == '"')
>  	    dquote = true;
>  	}
>  
>
> base-commit: 0a153c58a0ab68c6fa349d2ad0bf6a42e043ab23
> -- 
> 2.51.0

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

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

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

Literally after hitting send, it occurred to me, is the problem maybe
these two lines:

  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
    ++input;

And the other one in the 'else' block?  I think if *(input + 1) is '\0',
then the strchr call will return non-nullptr, which wasn't the desired
behaviour, and could result in stepping outside the string.

If this is the case then I think the correct fix would be checking if
the character at 'input + 1' is NULL or not, see the possible patch
below (there's no commit message or anything for it yet).

Thanks,
Andrew

---

diff --git i/gdb/infcmd.c w/gdb/infcmd.c
index 875bbe1ee69..7dd3392c96a 100644
--- i/gdb/infcmd.c
+++ w/gdb/infcmd.c
@@ -148,7 +148,9 @@ args_complete_p (const std::string &args)
 	     and we don't skip the entire '\\' then we'll only skip the
 	     first '\', in which case we might see the second '\' as a '\"'
 	     sequence, which would be wrong.  */
-	  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
+	  if (*input == '\\'
+	      && *(input + 1) != '\0'
+	      && strchr ("\"\\", *(input + 1)) != nullptr)
 	    ++input;
 	  /* Otherwise, just look for the closing double quote.  */
 	  else if (*input == '"')
@@ -162,7 +164,9 @@ args_complete_p (const std::string &args)
 	     a quoted argument.  The '\\' we need to skip so we don't just
 	     skip the first '\' and then incorrectly consider the second
 	     '\' are part of a '\"' or '\'' sequence.  */
-	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
+	  if (*input == '\\'
+	      && *(input + 1) != '\0'
+	      && strchr ("\"\\'", *(input + 1)) != nullptr)
 	    ++input;
 	  /* Otherwise, check for the start of a single or double quoted
 	     argument.  Single quotes have no special meaning on Windows

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Sorry to be a bore, but after reading this commit, and the bug report,
Andrew> it's still not obvious to me where the overflow actually occurs.

I believe in this code:

	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
	    ++input;

if *input == '\\' but this is also the last character of the string,
then strchr will return the address of the \0, then ++input will advance
past it.

I think sticking "&& input[1] != '\0'" in there might be sufficient.

Though perhaps there's also an issue if the string ends with spaces,
because:

      input = skip_spaces (input);
...
      ++input;

Tom

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

[Tom de Vries]

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

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;

        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;
        ++input;
      }

...
would catch both, I think.  Not that I'm suggesting this fix.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
> 
> 
>>
>> Also:
>> - factor out char array accesses using new variables c and next_c, and
>> - check for end-of-string after skip_spaces.
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754
>> ---
>>   gdb/infcmd.c | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 875bbe1ee69..ceacfd05683 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -126,17 +126,27 @@ static bool
>>   args_complete_p (const std::string &args)
>>   {
>>     const char *input = args.c_str ();
>> +  const char *end = input + args.length ();
>>     bool squote = false, dquote = false;
>>   
>> -  while (*input != '\0')
>> +  auto at = [&] (const char *s)
>> +    {
>> +      return s > end ? '\0' : *s;
>> +    };
>> +
>> +  while (at (input) != '\0')
>>       {
>>         input = skip_spaces (input);
>> +      char c = at (input);
>> +      if (c == '\0')
>> +	break;
>> +      char next_c = at (input + 1);
>>   
>>         if (squote)
>>   	{
>>   	  /* Inside a single quoted argument, look for the closing single
>>   	     quote.  */
>> -	  if (*input == '\'')
>> +	  if (c == '\'')
>>   	    squote = false;
>>   	}
>>         else if (dquote)
>> @@ -148,10 +158,10 @@ args_complete_p (const std::string &args)
>>   	     and we don't skip the entire '\\' then we'll only skip the
>>   	     first '\', in which case we might see the second '\' as a '\"'
>>   	     sequence, which would be wrong.  */
>> -	  if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
>> +	  if (c == '\\' && strchr ("\"\\", next_c) != nullptr)
>>   	    ++input;
>>   	  /* Otherwise, just look for the closing double quote.  */
>> -	  else if (*input == '"')
>> +	  else if (c == '"')
>>   	    dquote = false;
>>   	}
>>         else
>> @@ -162,7 +172,7 @@ args_complete_p (const std::string &args)
>>   	     a quoted argument.  The '\\' we need to skip so we don't just
>>   	     skip the first '\' and then incorrectly consider the second
>>   	     '\' are part of a '\"' or '\'' sequence.  */
>> -	  if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
>> +	  if (c == '\\' && strchr ("\"\\'", next_c) != nullptr)
>>   	    ++input;
>>   	  /* Otherwise, check for the start of a single or double quoted
>>   	     argument.  Single quotes have no special meaning on Windows
>> @@ -170,10 +180,10 @@ args_complete_p (const std::string &args)
>>   	     host to determine what is, or isn't a special character, when
>>   	     really, this is a function of the target.  */
>>   #ifndef _WIN32
>> -	  else if (*input == '\'')
>> +	  else if (c == '\'')
>>   	    squote = true;
>>   #endif
>> -	  else if (*input == '"')
>> +	  else if (c == '"')
>>   	    dquote = true;
>>   	}
>>   
>>
>> base-commit: 0a153c58a0ab68c6fa349d2ad0bf6a42e043ab23
>> -- 
>> 2.51.0
> 

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

[Tom de Vries]

On 1/6/26 9:47 AM, Tom de Vries wrote:
> 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 == " ".

I've written a unit test that reproduces the problem with args == " ".

Thanks,
- Tom

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 1a7daf1461b..88948a343da 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -56,6 +56,7 @@
  #include <optional>
  #include "source.h"
  #include "cli/cli-style.h"
+#include "gdbsupport/selftest.h"

  /* Local functions: */

@@ -123,7 +124,7 @@ show_inferior_tty_command (struct ui_file *file, int 
from_tty,
     finished.  */

  static bool
-args_complete_p (const std::string &args)
+args_complete_p (const std::string &args, const char **end = nullptr)
  {
    const char *input = args.c_str ();
    bool squote = false, dquote = false;
@@ -180,9 +185,28 @@ args_complete_p (const std::string &args)
        ++input;
      }

+  if (end != nullptr)
+    *end = input;
    return (!dquote && !squote);
  }

+#if GDB_SELF_TEST
+namespace selftests {
+
+static void
+infcmd_args_complete_p_tests (void)
+{
+  const char *end;
+
+  /* Regression test for heap-buffer-overflow reported in PR33754.  */
+  std::string s1 = " ";
+  SELF_CHECK (args_complete_p (s1, &end));
+  SELF_CHECK (end == s1.data () + s1.size ());
+}
+
+} /* namespace selftests */
+#endif /* GDB_SELF_TEST */
+
  /* Build a complete inferior argument string (all arguments to pass to the
     inferior) and return it.  ARGS is the initial part of the inferior
     arguments string, which might be the complete inferior arguments, in
@@ -3634,4 +3658,9 @@ Show whether `finish' prints the return value."), 
nullptr,
  			   nullptr,
  			   show_print_finish,
  			   &setprintlist, &showprintlist);
+
+#if GDB_SELF_TEST
+  selftests::register_test ("infcmd-args-complete-p",
+			    selftests::infcmd_args_complete_p_tests);
+#endif
  }

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

[Tom de Vries]

On 1/5/26 5:38 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +  auto at = [&] (const char *s)
> Tom> +    {
> Tom> +      return s > end ? '\0' : *s;
> Tom> +    };
> 
> I think it would better to avoid stepping off the end at the points
> where the pointer is incremented.
> 
> Tom> +  while (at (input) != '\0')
> 
> Like this could be
> 
>     for (input = skip_spaces (input); *input != '\0'; input = skip_spaces (input))
> 
> and then there's like one or two spots to check in the loop.

I've submitted a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2026-January/223715.html ). 
  I didn't do the transformation you suggested here, I tried to do 
something minimal.

I could submit a follow-up patch to do some refactoring in this 
function, though I'd likely do something like:
...
       while (true)
         {
           input = skip_spaces (input);
           if (*input == '\0')
              break;
...
instead of the for loop you're suggesting.

Thanks,
- Tom

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

[Tom de Vries]

On 1/6/26 9:47 AM, Tom de Vries wrote:
> 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 == " ".
> 
> 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;
> 
>         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;
>         ++input;
>       }
> 
> ...
> would catch both, I think.  Not that I'm suggesting this fix.
> 

I've submitted a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2026-January/223715.html ) 
handling both problems explicitly.

Thanks,
- Tom

> Thanks,
> - Tom
> 
>> Thanks,
>> Andrew
>>
>>
>>
>>
>>>
>>> Also:
>>> - factor out char array accesses using new variables c and next_c, and
>>> - check for end-of-string after skip_spaces.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754
>>> ---
>>>   gdb/infcmd.c | 24 +++++++++++++++++-------
>>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>>> index 875bbe1ee69..ceacfd05683 100644
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -126,17 +126,27 @@ static bool
>>>   args_complete_p (const std::string &args)
>>>   {
>>>     const char *input = args.c_str ();
>>> +  const char *end = input + args.length ();
>>>     bool squote = false, dquote = false;
>>> -  while (*input != '\0')
>>> +  auto at = [&] (const char *s)
>>> +    {
>>> +      return s > end ? '\0' : *s;
>>> +    };
>>> +
>>> +  while (at (input) != '\0')
>>>       {
>>>         input = skip_spaces (input);
>>> +      char c = at (input);
>>> +      if (c == '\0')
>>> +    break;
>>> +      char next_c = at (input + 1);
>>>         if (squote)
>>>       {
>>>         /* Inside a single quoted argument, look for the closing single
>>>            quote.  */
>>> -      if (*input == '\'')
>>> +      if (c == '\'')
>>>           squote = false;
>>>       }
>>>         else if (dquote)
>>> @@ -148,10 +158,10 @@ args_complete_p (const std::string &args)
>>>            and we don't skip the entire '\\' then we'll only skip the
>>>            first '\', in which case we might see the second '\' as a 
>>> '\"'
>>>            sequence, which would be wrong.  */
>>> -      if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
>>> +      if (c == '\\' && strchr ("\"\\", next_c) != nullptr)
>>>           ++input;
>>>         /* Otherwise, just look for the closing double quote.  */
>>> -      else if (*input == '"')
>>> +      else if (c == '"')
>>>           dquote = false;
>>>       }
>>>         else
>>> @@ -162,7 +172,7 @@ args_complete_p (const std::string &args)
>>>            a quoted argument.  The '\\' we need to skip so we don't just
>>>            skip the first '\' and then incorrectly consider the second
>>>            '\' are part of a '\"' or '\'' sequence.  */
>>> -      if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
>>> +      if (c == '\\' && strchr ("\"\\'", next_c) != nullptr)
>>>           ++input;
>>>         /* Otherwise, check for the start of a single or double quoted
>>>            argument.  Single quotes have no special meaning on Windows
>>> @@ -170,10 +180,10 @@ args_complete_p (const std::string &args)
>>>            host to determine what is, or isn't a special character, when
>>>            really, this is a function of the target.  */
>>>   #ifndef _WIN32
>>> -      else if (*input == '\'')
>>> +      else if (c == '\'')
>>>           squote = true;
>>>   #endif
>>> -      else if (*input == '"')
>>> +      else if (c == '"')
>>>           dquote = true;
>>>       }
>>>
>>> base-commit: 0a153c58a0ab68c6fa349d2ad0bf6a42e043ab23
>>> -- 
>>> 2.51.0
>>
> 

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

[Andrew Burgess]

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

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

[Tom de Vries]

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

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

[Andrew Burgess]

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

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

[Tom Tromey]

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

What would it take?

What I'd like ideally is that landing patches be done via the UI.
That way we could have some guarantee that what lands is what is
reviewed.  I don't think this is possible with the current setup though.

For my part I think my main worry is that I'd miss patches that I want
to be involved in.

Anyway it seems clear that the current approach isn't working extremely
well.  I don't know about anyone else but I have >3500 "pending"
messages in gdb-patches, some dating back years.  And probably more than
half of those are actual unreviewed patches.

Tom

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

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> Andrew> Maybe one day we'll move off the mailing list model.
>
> What would it take?

I cannot imagine a time in the near future when everyone involved in GDB
would agree to such a change.  But maybe if the majority of the largest
contributors wanted the change then we could figure something out?

>
> What I'd like ideally is that landing patches be done via the UI.
> That way we could have some guarantee that what lands is what is
> reviewed.  I don't think this is possible with the current setup though.

This would be my ideal too.  This opens the door real automated
pre-commit testing.  There are setups which do this sort of thing with a
mailing list (if I remember correctly), but it still relies on people
manually checking prior to pushing.

> For my part I think my main worry is that I'd miss patches that I want
> to be involved in.

True.  But at least for me, that's already the case as....

> Anyway it seems clear that the current approach isn't working extremely
> well.  I don't know about anyone else but I have >3500 "pending"
> messages in gdb-patches, some dating back years.  And probably more than
> half of those are actual unreviewed patches.

...I too have a huge backlog of unread patches.  I am trying to do more
reviews, but I just cannot read patches fast enough.  Anyway, I haven't
investigated, but my ideal system would be managed via a web UI, but
could still send users email when new patches are posted.  That way, if
folks find it easier to track their todo list via email, they could
still do that.

Thanks,
Andrew