Discussion:
[committed, PATCH] Check file size before getting section contents
(too old to reply)
H.J. Lu
2017-06-26 16:31:40 UTC
Permalink
Don't check the section size in bfd_get_full_section_contents since
the size of a decompressed section may be larger than the file size.
Instead, check file size in _bfd_generic_get_section_contents.

PR binutils/21665
* compress.c (bfd_get_full_section_contents): Don't check the
file size here.
* libbfd.c (_bfd_generic_get_section_contents): Check for and
reject a section whoes size + offset is greater than the size
of the entire file.
(_bfd_generic_get_section_contents_in_window): Likewise.
---
bfd/ChangeLog | 10 +++++++++-
bfd/compress.c | 8 +-------
bfd/libbfd.c | 17 ++++++++++++++++-
3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index ee926c6..e088cdc 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,4 +1,12 @@
-2017-06-26 Maciej W. Rozycki <***@imgtec.com>
+2017-06-26 H.J. Lu <***@intel.com>
+
+ PR binutils/21665
+ * compress.c (bfd_get_full_section_contents): Don't check the
+ file size here.
+ * libbfd.c (_bfd_generic_get_section_contents): Check for and
+ reject a section whoes size + offset is greater than the size
+ of the entire file.
+ (_bfd_generic_get_section_contents_in_window): Likewise.

2017-06-26 Nick Clifton <***@redhat.com>

diff --git a/bfd/compress.c b/bfd/compress.c
index 7b2c37c..ef549f9 100644
--- a/bfd/compress.c
+++ b/bfd/compress.c
@@ -239,12 +239,6 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
*ptr = NULL;
return TRUE;
}
- else if (bfd_get_file_size (abfd) > 0
- && sz > (bfd_size_type) bfd_get_file_size (abfd))
- {
- *ptr = NULL;
- return FALSE;
- }

switch (sec->compress_status)
{
@@ -260,7 +254,7 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
/* xgettext:c-format */
(_("error: %B(%A) is too large (%#lx bytes)"),
abfd, sec, (long) sz);
- return FALSE;
+ return FALSE;
}
}

diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index 554234f..b903328 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -789,6 +789,7 @@ _bfd_generic_get_section_contents (bfd *abfd,
bfd_size_type count)
{
bfd_size_type sz;
+ file_ptr filesz;
if (count == 0)
return TRUE;

@@ -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)
{
bfd_set_error (bfd_error_invalid_operation);
return FALSE;
@@ -835,6 +843,7 @@ _bfd_generic_get_section_contents_in_window
{
#ifdef USE_MMAP
bfd_size_type sz;
+ file_ptr filesz;

if (count == 0)
return TRUE;
@@ -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 ();
+ }
if (offset + count > sz
+ || (section->filepos + offset + sz) > (bfd_size_type) filesz
|| ! bfd_get_file_window (abfd, section->filepos + offset, count, w,
TRUE))
return FALSE;
--
2.9.4
Pedro Alves
2017-06-26 21:48:42 UTC
Permalink
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?

Thanks,
Pedro Alves
Post by H.J. Lu
Don't check the section size in bfd_get_full_section_contents since
the size of a decompressed section may be larger than the file size.
Instead, check file size in _bfd_generic_get_section_contents.
PR binutils/21665
* compress.c (bfd_get_full_section_contents): Don't check the
file size here.
* libbfd.c (_bfd_generic_get_section_contents): Check for and
reject a section whoes size + offset is greater than the size
of the entire file.
(_bfd_generic_get_section_contents_in_window): Likewise.
Pedro Alves
2017-06-26 22:24:34 UTC
Permalink
Post by Pedro Alves
Hi H.J.,
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.)
$ 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?
@@ -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
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 <***@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. Lu
2017-06-26 22:53:09 UTC
Permalink
Post by Pedro Alves
Post by Pedro Alves
Hi H.J.,
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.)
$ 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?
@@ -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
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;
Post by Pedro Alves
I'm testing this.
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.
H.J. Lu
2017-06-26 23:25:58 UTC
Permalink
Post by H.J. Lu
Post by Pedro Alves
Post by Pedro Alves
Hi H.J.,
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.)
$ 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?
@@ -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
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.
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.
Pedro Alves
2017-06-26 23:52:10 UTC
Permalink
Post by Pedro Alves
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.
Sorry, our messages cross paths. I've already pushed my version
in; it's quite late here, and I wanted to unbreak gdb for the buildbots
and everyone else. Mine seems like a better check to me, since we're
about to read "count" bytes at "section->filepos + offset" just
after the check, but I don't really know what led to the
original change. It'd be useful if when when you propose
an alternative, you explain what's wrong with the original, btw.

In any case, the GDB testsuite takes around 5 mins to run
on any modern machine, and the tests that fail are pretty
generic tests that don't really depend on host. A "make
check" in the gdb dir should work for you.

Thanks,
Pedro Alves
Pedro Alves
2017-06-26 23:26:31 UTC
Permalink
Post by Pedro Alves
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.
Testing gas+ld+gdb passed. I've pushed it in.

Thanks,
Pedro Alves
H.J. Lu
2017-06-26 22:49:12 UTC
Permalink
Post by Pedro Alves
Hi H.J.,
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.)
$ 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?
@@ -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
if (filesz < 0)
like in _bfd_generic_get_section_contents?
I checked in this patch:

commit 1f473e3d0ad285195934e6a077c7ed32afe66437
Author: H.J. Lu <***@gmail.com>
Date: Mon Jun 26 15:47:16 2017 -0700

Add a missing line to _bfd_generic_get_section_contents_in_window

PR binutils/21665
* libbfd.c (_bfd_generic_get_section_contents_in_window): Add
a missing line.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index cd5cb57..fbe2049 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,9 @@
+2017-06-26 H.J. Lu <***@intel.com>
+
+ PR binutils/21665
+ * libbfd.c (_bfd_generic_get_section_contents_in_window): Add
+ a missing line.
+
2017-06-26 Maciej W. Rozycki <***@imgtec.com>

* cpu-mips.c (arch_info_struct): Mark the 4010 32-bit.
diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index b903328..7b270ef 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -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 ();
Post by Pedro Alves
Thanks,
Pedro Alves
Post by H.J. Lu
Don't check the section size in bfd_get_full_section_contents since
the size of a decompressed section may be larger than the file size.
Instead, check file size in _bfd_generic_get_section_contents.
PR binutils/21665
* compress.c (bfd_get_full_section_contents): Don't check the
file size here.
* libbfd.c (_bfd_generic_get_section_contents): Check for and
reject a section whoes size + offset is greater than the size
of the entire file.
(_bfd_generic_get_section_contents_in_window): Likewise.
--
H.J.
Alan Modra
2017-06-26 23:15:02 UTC
Permalink
Post by H.J. Lu
filesz = bfd_get_file_size (abfd);
+ if (filesz < 0)
{
/* This should never happen. */
abort ();
This will abort for 2G files on some host/target combinations. Why is
that correct?
--
Alan Modra
Australia Development Lab, IBM
H.J. Lu
2017-06-26 23:27:27 UTC
Permalink
Post by Alan Modra
Post by H.J. Lu
filesz = bfd_get_file_size (abfd);
+ if (filesz < 0)
{
/* This should never happen. */
abort ();
This will abort for 2G files on some host/target combinations. Why is
that correct?
That is true. The problem is

file_ptr
bfd_get_size (bfd *abfd)
{
struct stat buf;

if (abfd->iovec == NULL)
return 0;

if (abfd->iovec->bstat (abfd, &buf) != 0)
return 0;

return buf.st_size;
}

Why isn't it "ufile_ptr".
--
H.J.
Alan Modra
2017-06-27 00:15:59 UTC
Permalink
Post by H.J. Lu
Post by Alan Modra
Post by H.J. Lu
filesz = bfd_get_file_size (abfd);
+ if (filesz < 0)
{
/* This should never happen. */
abort ();
This will abort for 2G files on some host/target combinations. Why is
that correct?
That is true. The problem is
file_ptr
bfd_get_size (bfd *abfd)
{
struct stat buf;
if (abfd->iovec == NULL)
return 0;
if (abfd->iovec->bstat (abfd, &buf) != 0)
return 0;
return buf.st_size;
}
Why isn't it "ufile_ptr".
I'm not sure of the history. However, the question of bfd_get_size
return type being signed isn't that relevant. What matters more is
the type used in the functions you patched, and that should be
unsigned, and the aborts removed. Also,
_bfd_generic_get_section_contents_in_window has the same problem that
Pedro fixed for _bfd_generic_get_section_contents.

PR binutils/21665
* libbfd.c (_bfd_generic_get_section_contents): Delete abort.
Use unsigned file pointer type, and remove cast.
* libbfd.c (_bfd_generic_get_section_contents_in_window): Likewise.
Add "count", not "sz".

diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index b8c65b5..0776451 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -789,7 +789,7 @@ _bfd_generic_get_section_contents (bfd *abfd,
bfd_size_type count)
{
bfd_size_type sz;
- file_ptr filesz;
+ ufile_ptr filesz;
if (count == 0)
return TRUE;

@@ -813,14 +813,9 @@ _bfd_generic_get_section_contents (bfd *abfd,
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
- || (section->filepos + offset + count) > (bfd_size_type) filesz)
+ || section->filepos + offset + count > filesz)
{
bfd_set_error (bfd_error_invalid_operation);
return FALSE;
@@ -843,7 +838,7 @@ _bfd_generic_get_section_contents_in_window
{
#ifdef USE_MMAP
bfd_size_type sz;
- file_ptr filesz;
+ ufile_ptr filesz;

if (count == 0)
return TRUE;
@@ -877,13 +872,8 @@ _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 ();
- }
if (offset + count > sz
- || (section->filepos + offset + sz) > (bfd_size_type) filesz
+ || section->filepos + offset + count > filesz
|| ! bfd_get_file_window (abfd, section->filepos + offset, count, w,
TRUE))
return FALSE;
--
Alan Modra
Australia Development Lab, IBM
Continue reading on narkive:
Loading...