[RFC,8.3,3/3] Avoid a crash in source_cache::extract_lines

  If the first requested line is larger than the number of lines in the
source buffer, source_cache::extract_lines could crash, because it
would try to pass string::npos" to string::substr.

This patch avoids the crash by checking for this case.

gdb/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* source-cache.c (source_cache::extract_lines): Handle case where
	first_pos==npos.
---
 gdb/ChangeLog      | 5 +++++
 gdb/source-cache.c | 2 ++
 2 files changed, 7 insertions(+)
  

On 03/08/2019 09:04 PM, Tom Tromey wrote:
> If the first requested line is larger than the number of lines in the
> source buffer, source_cache::extract_lines could crash, because it
> would try to pass string::npos" to string::substr.
> 
> This patch avoids the crash by checking for this case.

Can you clarify how can first_pos end up as npos?  Is that a bug in the
caller, or is it normal?  The documentation doesn't seem to allow for that:

  /* Get the source text for the source file in symtab S.  FIRST_LINE
     and LAST_LINE are the first and last lines to return; line
     numbers are 1-based.  If the file cannot be read, false is
     returned.  Otherwise, LINES_OUT is set to the desired text.  The
     returned text may include ANSI terminal escapes.  */

> 
> gdb/ChangeLog
> 2019-03-08  Tom Tromey  <tromey@adacore.com>
> 
> 	* source-cache.c (source_cache::extract_lines): Handle case where
> 	first_pos==npos.
> ---
>  gdb/ChangeLog      | 5 +++++
>  gdb/source-cache.c | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 27a0ade959c..b5d0d6cb7fc 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -98,6 +98,8 @@ source_cache::extract_lines (const struct source_text &text, int first_line,
>  	{
>  	  if (pos == std::string::npos)
>  	    pos = text.contents.size ();
> +	  if (first_pos == std::string::npos)
> +	    first_pos = text.contents.size ();
>  	  *lines = text.contents.substr (first_pos, pos - first_pos);
>  	  return true;
>  	}
> 

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Can you clarify how can first_pos end up as npos?  Is that a bug in the
Pedro> caller, or is it normal?  The documentation doesn't seem to allow for that:

Pedro>   /* Get the source text for the source file in symtab S.  FIRST_LINE
Pedro>      and LAST_LINE are the first and last lines to return; line
Pedro>      numbers are 1-based.  If the file cannot be read, false is
Pedro>      returned.  Otherwise, LINES_OUT is set to the desired text.  The
Pedro>      returned text may include ANSI terminal escapes.  */

I think you're just confusing first_pos and first_line here.
first_pos is a local variable that's used to track the position where
the first line starts:

  std::string::size_type first_pos = std::string::npos;
[...]
      if (lineno == first_line)
	first_pos = pos;

It can still be npos if first_line is greater than the number of lines
in the file.

Tom
  

On 03/13/2019 05:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Can you clarify how can first_pos end up as npos?  Is that a bug in the
> Pedro> caller, or is it normal?  The documentation doesn't seem to allow for that:
> 
> Pedro>   /* Get the source text for the source file in symtab S.  FIRST_LINE
> Pedro>      and LAST_LINE are the first and last lines to return; line
> Pedro>      numbers are 1-based.  If the file cannot be read, false is
> Pedro>      returned.  Otherwise, LINES_OUT is set to the desired text.  The
> Pedro>      returned text may include ANSI terminal escapes.  */
> 
> I think you're just confusing first_pos and first_line here.
> first_pos is a local variable that's used to track the position where
> the first line starts:
> 
>   std::string::size_type first_pos = std::string::npos;
> [...]
>       if (lineno == first_line)
> 	first_pos = pos;
> 
> It can still be npos if first_line is greater than the number of lines
> in the file.

Oh, I see now, now that I actually look at the code, rather than just the
patch.  Sorry about that.  And now that I look, I admit it took me a bit
to grok the function, but I got it.

IIUC, the function can never really return false, right?  Since
get_source_lines already validates input.  If you made extract_lines
return std::string instead of using an output parameter, then you
could conveniently write:

	  if (first_pos == std::string::npos)
	    return {};

for this case, which might be a little clearer than the
resulting "npos - npos" with your patch.

Anyhow, not that important.

Patch LGTM.

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> IIUC, the function can never really return false, right?  Since
Pedro> get_source_lines already validates input.  If you made extract_lines
Pedro> return std::string instead of using an output parameter,

I went ahead and made this change.

Tom