RE: [patch] Fix crash in read_pe_exported_syms

Pierre Muller
2013-03-02T15:31:22Z

I am not in the position of approving the patch,
but I must confess that this error is probably due to
the change I committed for this file (rev 1.19):
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/coff-pe-read.c?cvsroot=src

I still didn't really get a fully correct picture of the correct way to handle
the cleanups, but I think that your analysis is correct and that this patch should
be approved by a global maintainer.

This error probably explains partly the long thread about
crashes that we got with my patch for a while and that retarded its inclusion...
We probably fixed only part of the issues...

Thanks for finding this out,
and sorry for the troubles.

Pierre Muller

PS: It would also be better that this goes in before branching 7.6!

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Corinna Vinschen
> Envoyé : samedi 2 mars 2013 12:02
> À : gdb-patches@sourceware.org
> Objet : [patch] Fix crash in read_pe_exported_syms
>
> Hi,
>
> when running GDB from current CVS on a PE/COFF target, and if this
> target has no debug symbols, nor any exported symbols, then GDB crashes
> with a SEGV in the first do_cleanup called from coff_symfile_read.
>
> The reason is that read_pe_exported_syms creates two cleanup handlers,
> one of them referring to a symbol on the local stack:
>
> struct read_pe_section_data *section_data;
> [...]
> section_data = xzalloc (...)
> make_cleanup (free_current_contents, &section_data);
>
> but then returns from the function early in three different scenarios
> without calling do_cleanup. The subsequent do_cleanup call in
> coff_symfile_read now tries to dereference from an invalid stack address
> and ultimately crashes.
>
> Below is a patch. Ok to apply?
>
>
> Thanks,
> Corinna
>
>
> * coff-pe-read.c (read_pe_exported_syms): Don't return without
> calling do_cleanup.
>
>
> Index: coff-pe-read.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/coff-pe-read.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 coff-pe-read.c
> --- coff-pe-read.c 1 Jan 2013 06:32:40 -0000 1.23
> +++ coff-pe-read.c 2 Mar 2013 11:00:42 -0000
> @@ -379,7 +379,7 @@ read_pe_exported_syms (struct objfile *o
> /* This is not a recognized PE format file. Abort now, because
> the code is untested on anything else. *FIXME* test on
> further architectures and loosen or remove this test. */
> - return;
> + goto cleanup;
> }
>
> /* Get pe_header, optional header and numbers of export entries. */
> @@ -392,7 +392,7 @@ read_pe_exported_syms (struct objfile *o
>
> if (num_entries < 1) /* No exports. */
> {
> - return;
> + goto cleanup;
> }
> if (is_pe64)
> {
> @@ -448,7 +448,7 @@ read_pe_exported_syms (struct objfile *o
> if (export_size == 0)
> {
> /* Empty export table. */
> - return;
> + goto cleanup;
> }
>
> /* Scan sections and store the base and size of the relevant
> @@ -614,6 +614,7 @@ read_pe_exported_syms (struct objfile *o
> fprintf_unfiltered (gdb_stdlog, _("Finished reading \"%s\", exports
> %ld,"
> " forwards %ld, total %ld/%ld.\n"), dll_name, nbnormal,
> nbforward, nbnormal + nbforward, nexp);
> +cleanup:
> /* Discard expdata and section_data. */
> do_cleanups (back_to);
> }
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat