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

Andrew Burgess
2026-01-05T19:59:48Z

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