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

Tom de Vries
2026-01-06T08:47:44Z

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
>