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

[Andrew Burgess <aburgess@redhat.com>]

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