This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [committed, PATCH] Check file size before getting section contents
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;
> 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 ();
>
--
H.J.