Re: [committed, PATCH] Check file size before getting section contents

[Pedro Alves <palves at redhat dot com>]

On 06/26/2017 10:48 PM, Pedro Alves wrote:
> Hi H.J.,
> 
> This patch caused many regressions in GDB's testsuite:
> 
>   https://sourceware.org/ml/gdb-testers/2017-q2/msg05045.html
> 
> x86 buildslaves haven't caught up with the patch yet, but I
> see the same thing locally, on my x86-64 machine.  (git bisect
> pointed me here.)
> 
> Here's quick way to run only a few of the tests that are now failing:
> 
>  $ make check TESTS="gdb.base/break-interp.exp gdb.base/corefile.exp gdb.base/solib-search.exp gdb.reverse/sigall-precsave.exp"
> 
> Any idea what's going on?
> 
> BTW, this looks very suspicious:
> 
>  @@ -867,7 +876,13 @@ _bfd_generic_get_section_contents_in_window
>       sz = section->rawsize;
>     else
>       sz = section->size;
>  +  filesz = bfd_get_file_size (abfd);
>  +    {
>  +      /* This should never happen.  */
>  +      abort ();
>  +    }
> 
> ... because that abort is unconditional.  Did you intend
> to guard it with:
>    if (filesz < 0)
> like in _bfd_generic_get_section_contents?
> 

> @@ -811,8 +812,15 @@ _bfd_generic_get_section_contents (bfd *abfd,
>      sz = section->rawsize;
>    else
>      sz = section->size;
> +  filesz = bfd_get_file_size (abfd);
> +  if (filesz < 0)
> +    {
> +      /* This should never happen.  */
> +      abort ();
> +    }
>    if (offset + count < count
> -      || offset + count > sz)
> +      || offset + count > sz
> +      || (section->filepos + offset + sz) > (bfd_size_type) filesz)
>      {

The problem is this new "section->filepos + offset + sz"
check here.  GDB calls bfd_get_section_contents with offset != 0,
which causes that "offset + sz" addition to shoot past filesz.
I can't see how that new check makes sense as is.  We're reading
"count" bytes, not "sz" bytes.

I'm testing this.

From: Pedro Alves <palves@redhat.com>
Date: 2017-06-26 22:51:02 +0100

Fix
---

 bfd/libbfd.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index b903328..b8c65b5 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -820,7 +820,7 @@ _bfd_generic_get_section_contents (bfd *abfd,
     }
   if (offset + count < count
       || offset + count > sz
-      || (section->filepos + offset + sz) > (bfd_size_type) filesz)
+      || (section->filepos + offset + count) > (bfd_size_type) filesz)
     {
       bfd_set_error (bfd_error_invalid_operation);
       return FALSE;
@@ -877,6 +877,7 @@ _bfd_generic_get_section_contents_in_window
   else
     sz = section->size;
   filesz = bfd_get_file_size (abfd);
+  if (filesz < 0)
     {
       /* This should never happen.  */
       abort ();