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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Jun 26, 2017 at 3:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jun 26, 2017 at 3:24 PM, Pedro Alves <palves@redhat.com> wrote:
>> 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.
>
> Please try this:
>
> diff --git a/bfd/libbfd.c b/bfd/libbfd.c
> index 7b270ef..8bad4ab 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 + sz) > (bfd_size_type) filesz)
>      {
>        bfd_set_error (bfd_error_invalid_operation);
>        return FALSE;
> @@ -883,7 +883,7 @@ _bfd_generic_get_section_contents_in_window
>        abort ();
>      }
>    if (offset + count > sz
> -      || (section->filepos + offset + sz) > (bfd_size_type) filesz
> +      || (section->filepos + sz) > (bfd_size_type) filesz
>        || ! bfd_get_file_window (abfd, section->filepos + offset, count, w,
>   TRUE))
>      return FALSE;
>

With changelog.



-- 
H.J.

From 023539992080d3c20053ecb768a8869a4db3d635 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 26 Jun 2017 16:08:15 -0700
Subject: [PATCH] Properly check section size in
 _bfd_generic_get_section_contents

Check

(section->filepos + sz) > file size

instead of

(section->filepos + offset + sz) > file size

	* libbfd.c (_bfd_generic_get_section_contents): Don't add
	section offset when checking section size agains file size.
	(_bfd_generic_get_section_contents_in_window): Likewise.
---
 bfd/libbfd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index 7b270ef..8bad4ab 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 + sz) > (bfd_size_type) filesz)
     {
       bfd_set_error (bfd_error_invalid_operation);
       return FALSE;
@@ -883,7 +883,7 @@ _bfd_generic_get_section_contents_in_window
       abort ();
     }
   if (offset + count > sz
-      || (section->filepos + offset + sz) > (bfd_size_type) filesz
+      || (section->filepos + sz) > (bfd_size_type) filesz
       || ! bfd_get_file_window (abfd, section->filepos + offset, count, w,
 				TRUE))
     return FALSE;
-- 
2.9.4