[patch] Fix crash in read_pe_exported_syms

[patch] Fix crash in read_pe_exported_syms

[Corinna Vinschen]

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

RE: [patch] Fix crash in read_pe_exported_syms

[Pierre Muller]

  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

Re: [patch] Fix crash in read_pe_exported_syms

[Sergio Durigan Junior]

Hi Pierre,

On Saturday, March 02 2013, Pierre Muller wrote:

>   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.

Not sure if you meant that you want to understand how cleanups work, but
anyway...

Think of cleanups as a stack.  When you call `make_cleanup', you are
asking for a new operation to be put in the stack, and you receive the
pointer to the last operation in that stack (before yours, which becomes
the new head).  When you call `do_cleanups', you are telling GDB to
iterate over the stack in reverse and execute each operation until the
one you provide as the argument of the function.  `discard_cleanups'
does a similar job, but instead of executing each operation, it discards
them.

When you know that you will have many cleanups and don't want to keep
track of each one individually, you can use the same trick as
`read_pe_exported_syms': you register a dummy cleanup (null_cleanup,
which does nothing) and then start calling `make_cleanup' without caring
about its return value.  Then, when the time comes (e.g., you are about
to return from the function), you can either call `do_cleanups' or
`discard_cleanups' using as the argument the one you received in the
first call to `make_cleanup' with a dummy cleanup, and you'll be sure
that all the cleanup chain will be executed or discarded up to the point
where you started your function.

Sorry if you already knew all that and I misunderstood your question
:-).

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

I agree.

-- 
Sergio

Re: [patch] Fix crash in read_pe_exported_syms

[Sergio Durigan Junior]

On Saturday, March 02 2013, Corinna Vinschen wrote:

> Hi,

Hi Corinna,

> 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?

[I am not a maintainer.]

You are right, the function returns without taking care of the possible
cleanups.  IMO the patch is almost obvious, but thanks anyway for
sending it here.

I don't want to create a flamewar here, but ISTR that the use of `goto'
is discouraged in GDB/GNU.  I remember I submitted a patch once that
made use of `goto' in a similar way that you are doing here, and I was
asked to rewrite it.

Anyway, recently I faced a similar issue (i.e., having to call
`do_cleanup' in several places, and what I did was to actually call it
in all those places instead of using `goto'.  You might want to wait
until a global maintainer emits some opinion about this.

Thanks,

-- 
Sergio

Re: [patch] Fix crash in read_pe_exported_syms

[Pedro Alves]

On 03/02/2013 11:02 AM, Corinna Vinschen wrote:

(...)

I'm not myself against gotos, but indeed GDB's style leans
largely on the "call do_cleanups before return" for early
returns.

On that basis,

> Below is a patch.  Ok to apply?

OK with Sergio's suggestion.

-- 
Pedro Alves

Re: [patch] Fix crash in read_pe_exported_syms

[Corinna Vinschen]

On Mar  4 13:00, Pedro Alves wrote:
> On 03/02/2013 11:02 AM, Corinna Vinschen wrote:
> 
> (...)
> 
> I'm not myself against gotos, but indeed GDB's style leans
> largely on the "call do_cleanups before return" for early
> returns.

Well, grepping for "goto" in the code returns >500 places.  I checked
that before sending my patch...

> On that basis,
> 
> > Below is a patch.  Ok to apply?
> 
> OK with Sergio's suggestion.

I really think that lots of 

  do_cleanup();
  return;

blocks are more error prone and less readable than a goto to a defined
label at the end of a function.  Are you really sure you want that?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Re: [patch] Fix crash in read_pe_exported_syms

[Joel Brobecker]

> I really think that lots of 
> 
>   do_cleanup();
>   return;
> 
> blocks are more error prone and less readable than a goto to a defined
> label at the end of a function.  Are you really sure you want that?

Neither is obviously better, IMO, but the general style in GDB so far
has been to call the cleanups before returning...

-- 
Joel

Re: [patch] Fix crash in read_pe_exported_syms

[Corinna Vinschen]

On Mar  4 09:41, Joel Brobecker wrote:
> > I really think that lots of 
> > 
> >   do_cleanup();
> >   return;
> > 
> > blocks are more error prone and less readable than a goto to a defined
> > label at the end of a function.  Are you really sure you want that?
> 
> Neither is obviously better, IMO, but the general style in GDB so far
> has been to call the cleanups before returning...

Ok.  Applied with that change.


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat