Revert bfd_get_size checks
Alan Modra
amodra@gmail.com
Fri Nov 7 02:53:00 GMT 2014
On Thu, Nov 06, 2014 at 02:43:26PM +0000, Nicholas Clifton wrote:
> Hi Alan,
>
> >I think we should revert all of these bfd_get_size checks, given the
> >number of errors they have introduced, and that it's better to allow a
> >malloc, seek or read failure on a corrupt file than penalize good
> >files with a stat. Nick?
>
> OK, *sigh*. I introduced the checks in order to produce more helpful error
> messages when the BFD library encounters a corrupt file. At least that was
> the plan. It obviously has not worked out so well.
>
> I have already checked in one patch to revert some of the checks. Feel free
> to revert the rest of them though. I will rerun the binutils against all of
> the corrupt binaries afterwards and see if any real failures pop up.
Sorry to do this to you Nick, but they really do need reverting.
Here's why:
archive.c
(_bfd_slurp_extended_name_table): Doesn't take into account file
offset, and bfd_bread a little later will do the proper check.
coffcode.h
(coff_set_alignment_hook): Doesn't take into account file offset.
Also, why is this particular assignment to section->reloc_count worthy
of checking, while the one in coffgen.c:make_a_section_from_file is
not?
(coff_slurp_line_table): Compares internal data structure size
against external file size.
coffgen.c
(coff_get_normalized_symtab): Compares internal data structure size
against external file size.
(_bfd_coff_get_external_symbols): Doesn't take into account file
offset, and bfd_seek/bfd_bread a little later will do the proper
check.
elf.c
(bfd_elf_get_str_section): Doesn't take into account file offset and
bfd_read a little later will do the proper check.
tekhex.c
(first_phase): Section size has nothing to do with file size. The
huge amount of time spent is processing %1B3709T_SEGMENT1108FFFFFFFF,
which says we have a section called "T_SEGMENT1", starting at vma 0
and ending at vma 0xffffffff. This is processed one byte at a time in
tekhex.c:move_section_contents creating 8k chunks of memory every 8k
which are then stored on a linked list. So, huge amount of memory
consumed for contents implicitly zero, and a huge linked list
traversed. In my opinion it is a complete waste of time rewriting
tekhex.c to better deal with fuzzed input.
I recognize that saying "bfd_bread does the proper checks later" isn't
telling the whole story, since there is often a bfd_malloc or similar
call first. The alloc, especially where you've turned it into a
zalloc/zmalloc, has the potential to thrash your machine before
hitting an OOM..
I've committed the following.
* archive.c (_bfd_slurp_extended_name_table): Revert bfd_get_size check.
* coffcode.h (coff_set_alignment_hook): Likewise.
(coff_slurp_line_table): Likewise.
* coffgen.c (coff_get_normalized_symtab): Likewise.
(_bfd_coff_get_external_symbols): Likewise.
* elf.c (bfd_elf_get_str_section): Likewise.
* tekhex.c (first_phase): Likewise.
diff --git a/bfd/archive.c b/bfd/archive.c
index b905213..9e94745 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -1293,9 +1293,6 @@ _bfd_slurp_extended_name_table (bfd *abfd)
amt = namedata->parsed_size;
if (amt + 1 == 0)
goto byebye;
- /* PR binutils/17533: A corrupt archive can contain an invalid size. */
- if (amt > (bfd_size_type) bfd_get_size (abfd))
- goto byebye;
bfd_ardata (abfd)->extended_names_size = amt;
bfd_ardata (abfd)->extended_names = (char *) bfd_zalloc (abfd, amt + 1);
diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index 1ca28b8..3abb6a3 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -1919,15 +1919,6 @@ coff_set_alignment_hook (bfd * abfd ATTRIBUTE_UNUSED,
if (bfd_seek (abfd, oldpos, 0) != 0)
return;
section->reloc_count = hdr->s_nreloc = n.r_vaddr - 1;
- /* PR binutils/17512: Stop corrupt files from causing
- memory problems if they claim to have too many relocs. */
- if (section->reloc_count * relsz > (bfd_size_type) bfd_get_size (abfd))
- {
- (*_bfd_error_handler)
- ("%s: warning: claims to have %#x relocs, but the file is not that big",
- bfd_get_filename (abfd), section->reloc_count);
- section->reloc_count = 0;
- }
section->rel_filepos += relsz;
}
else if (hdr->s_nreloc == 0xffff)
@@ -4528,8 +4519,6 @@ coff_slurp_line_table (bfd *abfd, asection *asect)
BFD_ASSERT (asect->lineno == NULL);
amt = ((bfd_size_type) asect->lineno_count + 1) * sizeof (alent);
- if (amt > (bfd_size_type) bfd_get_size (abfd))
- return FALSE;
lineno_cache = (alent *) bfd_zalloc (abfd, amt);
if (lineno_cache == NULL)
return FALSE;
diff --git a/bfd/coffgen.c b/bfd/coffgen.c
index 4856a40..9ad0783 100644
--- a/bfd/coffgen.c
+++ b/bfd/coffgen.c
@@ -1619,14 +1619,6 @@ _bfd_coff_get_external_symbols (bfd *abfd)
if (size == 0)
return TRUE;
- /* PR binutils/17512: Do not even try to load
- a symbol table bigger than the entire file... */
- if (size >= (bfd_size_type) bfd_get_size (abfd))
- {
- fprintf (stderr, "XXX SIZE FAIL 1\n");
- return FALSE;
- }
-
syms = bfd_malloc (size);
if (syms == NULL)
return FALSE;
@@ -1759,16 +1751,7 @@ coff_get_normalized_symtab (bfd *abfd)
if (obj_raw_syments (abfd) != NULL)
return obj_raw_syments (abfd);
- size = obj_raw_syment_count (abfd);
- /* PR binutils/17512: Do not even try to load
- a symbol table bigger than the entire file...
- Note - we do not fail on a size of 0. Linker created
- bfds can have this property and they are not corrupt. */
- if (size >= (bfd_size_type) bfd_get_size (abfd)
- && bfd_get_size (abfd) > 0)
- return NULL;
-
- size *= sizeof (combined_entry_type);
+ size = obj_raw_syment_count (abfd) * sizeof (combined_entry_type);
internal = (combined_entry_type *) bfd_zalloc (abfd, size);
if (internal == NULL && size != 0)
return NULL;
diff --git a/bfd/elf.c b/bfd/elf.c
index 7cc0ce1..9c4dcdf 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -294,11 +294,6 @@ bfd_elf_get_str_section (bfd *abfd, unsigned int shindex)
offset = i_shdrp[shindex]->sh_offset;
shstrtabsize = i_shdrp[shindex]->sh_size;
- /* PR binutils/17512: Do not even try to load
- a string table bigger than the entire file... */
- if (shstrtabsize >= (bfd_size_type) bfd_get_size (abfd))
- return NULL;
-
/* Allocate and clear an extra byte at the end, to prevent crashes
in case the string table is not terminated. */
if (shstrtabsize + 1 <= 1
diff --git a/bfd/tekhex.c b/bfd/tekhex.c
index 85f5593..2220d50 100644
--- a/bfd/tekhex.c
+++ b/bfd/tekhex.c
@@ -403,9 +403,6 @@ first_phase (bfd *abfd, int type, char *src)
if (!getvalue (&src, &val))
return FALSE;
section->size = val - section->vma;
- /* PR binutils/17512: Make sure that the size is sane. */
- if (section->size > (bfd_size_type) bfd_get_size (abfd))
- return FALSE;
section->flags = SEC_HAS_CONTENTS | SEC_LOAD | SEC_ALLOC;
break;
case '0':
--
Alan Modra
Australia Development Lab, IBM
More information about the Binutils
mailing list