IIUC the problem is that sect->size comes from a header somewhere,
but does not reflect the true size of the section.  It took me a while
to understand this, because from the report I thought you meant that the
comparison could not possibly be true -- but really it could be ==.

I am not sure where the correct fix is.
I notice readelf gives a warning for this file:

readelf: /tmp/pr-23657: Warning: Size of section 31 is larger than the entire file!

So maybe gdb could do this check.
Or maybe BFD ought to issue an error because the file is corrupted.

CCing Nick - Nick, do you think this should be handled in BFD?

I couldn't reproduce directly but valgrind shows the error.

Hi Tom,
 
> I am not sure where the correct fix is.
> I notice readelf gives a warning for this file:

So does objdump, if you use an option which tries to reference
into the debug string section:

  % objdump -S objdump
  objdump: error: objdump(.debug_str) is too large (0xffffffffffffffff bytes)

> So maybe gdb could do this check.
> Or maybe BFD ought to issue an error because the file is corrupted.
> 
> CCing Nick - Nick, do you think this should be handled in BFD?

Both would be my suggestion.  You can never be too paranoid...

There is also the caveat that the test "if (str_offset >= sect->size)"
might be wrong if the .debug_str section is compressed.  (I am unfamiliar 
with the gdb sources, so I do not know if sect->size is the size from the 
ELF header, or the size after decompression). 

Cheers
  Nick

(In reply to Nick Clifton from comment #2)

> There is also the caveat that the test "if (str_offset >= sect->size)"
> might be wrong if the .debug_str section is compressed.  (I am unfamiliar 
> with the gdb sources, so I do not know if sect->size is the size from the 
> ELF header, or the size after decompression). 

For a compressed section it comes from:

  descriptor->size = bfd_get_section_size (sectp);

Where would a section size sanity check belong in BFD?
Maybe we'd only want it for sections we actually read, so
bfd_get_full_section_contents (and then a copy in gdb since
gdb can mmap sections).

I wonder if there are other sanity checks that should be done.

Hi Tom,

(In reply to Tom Tromey from comment #3)

> Where would a section size sanity check belong in BFD?

Mmmm, well I think that when the file is first read in would 
be the most reasonable place.  I am going to experiment with
a patch to update elf_swap_shdr_in (in bfd/elfcode.h) and see
how that fairs.

> Maybe we'd only want it for sections we actually read, so
> bfd_get_full_section_contents (and then a copy in gdb since
> gdb can mmap sections).

That would make sense, although I think that we whould also
issue a warning message (but not an error) if we detect
something funny going on when the section headers are read in.

Cheers
  Nick

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8ff71a9c80cfcf64c54d4ae938c644b1b1ea19fb

commit 8ff71a9c80cfcf64c54d4ae938c644b1b1ea19fb
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Sep 18 16:54:07 2018 +0100

    Add a warning to the bfd library for when it encounters an ELF file with an invalid section size.
    
    	PR 23657
    	* elfcode.h (elf_swap_shdr_in): Generate a warning message if an
    	ELF section has contents and size larger than the file size.

This should now be fixed.

(In reply to Keith Seitz from comment #6)
> This should now be fixed.

Is it?

I can't reproduce the sigsegv before and after the commit, but using valgrind I can get a "Invalid read of size 1" before and after the commit.

AFAIU, the commit just adds a warning, which indeed triggers. But the commit doesn't fix the out of bounds memory access.

(In reply to Tom de Vries from comment #7)
> (In reply to Keith Seitz from comment #6)
> > This should now be fixed.
> 
> Is it?

It does with the attached reproducer with 950b74950f6 (which doesn't
appear recorded here for some reason).

> I can't reproduce the sigsegv before and after the commit, but using
> valgrind I can get a "Invalid read of size 1" before and after the commit.
> 
> AFAIU, the commit just adds a warning, which indeed triggers. But the commit
> doesn't fix the out of bounds memory access.

The bfd patch recorded here does only add a warning. However, my commit
will emit another warning and skip the section entirely. There is no way
for the code reported by this bug to get triggered, i.e., the entire
.debug_str section will not be read.

That invalid read must be another bug -- one I did not know about.

(In reply to Keith Seitz from comment #8)
> (In reply to Tom de Vries from comment #7)
> > (In reply to Keith Seitz from comment #6)
> > > This should now be fixed.
> > 
> > Is it?
> 
> It does with the attached reproducer with 950b74950f6 (which doesn't
> appear recorded here for some reason).

The commit 950b74950f6 "DWARF reader: Reject sections with invalid sizes" mentions PR gdb/23567.  That looks like a typo.

> That invalid read must be another bug -- one I did not know about.

I have tested before and after commit 8ff71a9c80c, as I was not aware of another commit.

FTR: Fixed by: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=950b74950f6020eda38647f22e9077ac7f68ca49 :

DWARF reader: Reject sections with invalid sizes
author	Keith Seitz <keiths@redhat.com>	
	Wed, 16 Oct 2019 18:33:59 +0000 (11:33 -0700)
committer	Keith Seitz <keiths@redhat.com>	
	Wed, 16 Oct 2019 18:35:16 +0000 (11:35 -0700)
commit	950b74950f6020eda38647f22e9077ac7f68ca49
tree	6179c525842b477617cbb1b97965222454e69ae6	tree
parent	ff371ec99988662e16b061fe0f66e989340f129a	commit | diff
DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186: const

              8 *>(.:
                     ;'@�B);
747: const

              8 *�(.:
                     ;'@�B);
701: const

              8 *�D �
                     (.:
                        ;'@�B);
71: const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

PR gdb/23567
* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f

setting target milestone to 9.1 since the change was pushed to master prior to the gdb-9-branch being created.