[RFA] Fix a memory corruption in mdebugread.c

[RFA] Fix a memory corruption in mdebugread.c

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 5808 bytes --]

This problem is happening on Tru64 5.1A.

GDB crashed while reading the debugging information of an application
created by one of our customers. Their application is a mix of C++ and
Ada. In order to investigate, this customer gave us the binaries of
their application as we don't have a C++ compiler on Tru64. We managed
to find the source of the problem, and fix it (hopefuly :), but not
having a C++ compiler, we are not able to produce a testcase for it.

Here is what happened:

In their application, they had one class with a few constructors, one
destructor, and then some class variables. In mdebugread.c, GDB read
class types as TYPE_CODE_STRUCTs. For reasons probably related to the
ECOFF format (my knowledge of this format is still a bit partial), GDB
creates the struct type in several passes. 

In the first pass, when GDB encounters the struct type definition for
the first time, it does the following:
  - create a type object, saves the name of the type, and a few other info.
  - counts the number of fields in the struct, and allocates enough
    memory to holds these fields...

The problem in our case was that GDB was mis-counting (under-counting
actually) the number of fields of the C++ class. The method used in GDB
to do the counting is fairly simple. GDB knows that structs definitions
are started with a stBlock, that they end with a stEnd, and that all
stMember symbol records between the 2 markers are fields in the struct.
Simplifying it a bit the current counting code, we have:

     nfields = 0;
     ALL_SYMBOL_RECORDS (tsym)
       {
         if (tsym.st == stEnd)
           break;
         else if (tsym.st == stMember)
           nfields++;
       }

Unfortunately, in the case of C++ classes, the struct usually contains
methods which are encoded in something ressembling the following sequence
of symbol records:
     
     stProc
     stParam
     stParam
     [...]
     stEnd

The algorithm above is therefore not resistant to procedure definitions
nested in struct definitions, as GDB then ends the counting prematurely.

In our case, GDB stopped counting after finding 0 fields, and therefore
allocated 0 bytes for the struct fields:

        TYPE_FIELDS (t) = f = ((struct field *)
                               TYPE_ALLOC (t,
                                           nfields * sizeof (struct field)));


The trouble starts during the second pass, when we actually try to fill
in the info for the fields...

    case stMember:              /* member of struct or union */
      f = &TYPE_FIELDS (top_stack->cur_type)[top_stack->cur_field++];
      FIELD_NAME (*f) = name;
      FIELD_BITPOS (*f) = sh->value;
      bitsize = 0;
      FIELD_TYPE (*f) = parse_type (cur_fd, ax, sh->index, &bitsize, bigend, name);
      FIELD_BITSIZE (*f) = bitsize;
      break;

Notice how we don't check for the number of fields allocated before
accessing field number top_stack->cur_field... Because we did not
allocate enough space to store all fields, we end up with a buffer
overflow!

The memory for these fields was allocated on the obstack. In the time
interval between the moment when we allocated the 0 bytes for the
fields, and the moment we overflow, GDB did also allocate a new type
object, also on the same obstack. Understandably, the obstack placed
this new type at the same location as the fields. Argh!

While we happily store the information for each field, we corrupt the
data for the other type. It is only a bit later when we try to dereference
one of the fields in this corrupted type that GDB crashes with a SEGV...

The following patch is, I admit, a minimal attempt at fixing the
problem. It would probably be more complete to handle StProc symbol
records in the counting loop and skip the whole stProc sequence, just
as we do for stBlock et al:

              case stBlock:
              case [...]:
              case stStruct:
                {
                  if (tsym.index != 0)
                    {
                      /* This is something like a struct within a
                         struct.  Skip over the fields of the inner
                         struct.  The -1 is because the for loop will
                         increment ext_tsym.  */
                      ext_tsym = ((char *) debug_info->external_sym
                                  + ((cur_fdr->isymBase + tsym.index - 1)
                                     * external_sym_size));
                    }

Unfornately, I lack the time to do this. Instead, I did the following
trivial change, which just ignores any stEnd symbol records if they are
not the one ending the struct definition. This is done by matching the
symbol name associated to the stEnd SYMR against the name of the struct.

This change fixes the problem reported by our customer, and does not
introduce any regression in the testsuite. Unfortunately, we don't have
a C++ compiler, so the C++ part of the testsuite is inoperable for us,
and we could not test the effect of this change on C++.

Given my current analysis, this change seems sensible. I am therefore
recommending it for inclusion. If acceptable, we may also want to
include it in the 5.3 branch as well, as it fixes a crash. Any feedback
from somebody having a C++ compiler would be greatly appreciated. 

2002-12-31  J. Brobecker  <brobecker@gnat.com>

        * mdebugread.c (parse_symbol): Make sure to identify the correct
        stEnd symbol record while counting the number of fields when parsing
        the debugging information for a structure. Otherwise, GDB sometimes
        ends up under-counting the number of felds in the struct, and this
        causes later a memory corruption responsible for a GDB crash when
        running or attaching to the application.
        Fixes [B927-009]

Ok to commit?

Thanks,
-- 
Joel

[-- Attachment #2: mdebugread.c.diff --]
[-- Type: text/plain, Size: 1486 bytes --]

Index: mdebugread.c
===================================================================
RCS file: /cvs/src/src/gdb/mdebugread.c,v
retrieving revision 1.32
diff -c -3 -p -r1.32 mdebugread.c
*** mdebugread.c	17 Dec 2002 00:39:07 -0000	1.32
--- mdebugread.c	31 Dec 2002 14:09:25 -0000
*************** parse_symbol (SYMR *sh, union aux_ext *a
*** 865,871 ****
  	    switch (tsym.st)
  	      {
  	      case stEnd:
! 		goto end_of_fields;
  
  	      case stMember:
  		if (nfields == 0 && type_code == TYPE_CODE_UNDEF)
--- 865,883 ----
  	    switch (tsym.st)
  	      {
  	      case stEnd:
!                 /* C++ encodes class types as structures where there the
!                    methods are encoded as stProc. The scope of stProc
!                    symbols also ends with stEnd, thus creating a risk of
!                    taking the wrong stEnd symbol record as the end of
!                    the current struct, which would cause GDB to undercount
!                    the real number of fields in this struct.  To make sure
!                    we really reached the right stEnd symbol record, we
!                    check the associated name, and match it against the
!                    struct name.  */
!                 if (strcmp (debug_info->ss + cur_fdr->issBase + tsym.iss,
!                             name) == 0)
!                   goto end_of_fields;
!                 break;
  
  	      case stMember:
  		if (nfields == 0 && type_code == TYPE_CODE_UNDEF)

Re: [RFA] Fix a memory corruption in mdebugread.c

[Daniel Jacobowitz]

On Tue, Dec 31, 2002 at 06:15:20PM +0400, Joel Brobecker wrote:
> This problem is happening on Tru64 5.1A.
> 
> GDB crashed while reading the debugging information of an application
> created by one of our customers. Their application is a mix of C++ and
> Ada. In order to investigate, this customer gave us the binaries of
> their application as we don't have a C++ compiler on Tru64. We managed
> to find the source of the problem, and fix it (hopefuly :), but not
> having a C++ compiler, we are not able to produce a testcase for it.
> 
> Here is what happened:

[snip]

> The following patch is, I admit, a minimal attempt at fixing the
> problem. It would probably be more complete to handle StProc symbol
> records in the counting loop and skip the whole stProc sequence, just
> as we do for stBlock et al:

So I assume the stProc is nested directly in the type, and we generate
a complaint() in the default below?  It's worth bearing in mind that we
don't actually support C++ in mdebugread; debugging the C++ part won't
work well at all.  We won't recognize any member methods for instance.

> 
>               case stBlock:
>               case [...]:
>               case stStruct:
>                 {
>                   if (tsym.index != 0)
>                     {
>                       /* This is something like a struct within a
>                          struct.  Skip over the fields of the inner
>                          struct.  The -1 is because the for loop will
>                          increment ext_tsym.  */
>                       ext_tsym = ((char *) debug_info->external_sym
>                                   + ((cur_fdr->isymBase + tsym.index - 1)
>                                      * external_sym_size));
>                     }
> 
> Unfornately, I lack the time to do this. Instead, I did the following
> trivial change, which just ignores any stEnd symbol records if they are
> not the one ending the struct definition. This is done by matching the
> symbol name associated to the stEnd SYMR against the name of the struct.
> 
> This change fixes the problem reported by our customer, and does not
> introduce any regression in the testsuite. Unfortunately, we don't have
> a C++ compiler, so the C++ part of the testsuite is inoperable for us,
> and we could not test the effect of this change on C++.

My concern from your description is that a constructor may have the
same name as the enclosing type.  Are these mangled names?  Qualified
names?  Base names, in which case the constructor is a problem?  You
should be able to figure this out from looking at a couple of the names
found by the check below.

> Given my current analysis, this change seems sensible. I am therefore
> recommending it for inclusion. If acceptable, we may also want to
> include it in the 5.3 branch as well, as it fixes a crash. Any feedback
> from somebody having a C++ compiler would be greatly appreciated. 
> 
> 2002-12-31  J. Brobecker  <brobecker@gnat.com>
> 
>         * mdebugread.c (parse_symbol): Make sure to identify the correct
>         stEnd symbol record while counting the number of fields when parsing
>         the debugging information for a structure. Otherwise, GDB sometimes
>         ends up under-counting the number of felds in the struct, and this
>         causes later a memory corruption responsible for a GDB crash when
>         running or attaching to the application.
>         Fixes [B927-009]
> 
> Ok to commit?

That's not a proper ChangeLog entry; it should be smeting like:

2002-12-31  J. Brobecker  <brobecker@gnat.com>

	* mdebugread.c (parse_symbol): Count until the stEnd matching
	the structure name.


[Don't blame me, blame GNU....]


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix a memory corruption in mdebugread.c

[Joel Brobecker]

Hi Daniel,

> So I assume the stProc is nested directly in the type, and we generate
> a complaint() in the default below?

Right.

> It's worth bearing in mind that we don't actually support C++ in
> mdebugread; debugging the C++ part won't work well at all.  We won't
> recognize any member methods for instance.

This is indeed good to know, although I kind of guessed this after
reading the code in mdebugread.c :).

> My concern from your description is that a constructor may have the
> same name as the enclosing type.  Are these mangled names?  Qualified
> names?  Base names, in which case the constructor is a problem?  You
> should be able to figure this out from looking at a couple of the names
> found by the check below.

The method names are mangled, same for the constructors and destructors.
For instance, one of the constructors for class TC_rule was
__ct__7TC_ruleXRC7TC_rule. The destructor was __dt__7TC_ruleXv
(actually, I assumed these stProc entries are constructors and
destructors, but this a guess based on the name). An example of stProc
entry for a method: FI_write__7TC_ruleXP4FILE.

stMembers, on the other hand, are not mangled. But they don't come with
stEnd entries, so we should be safe.

> That's not a proper ChangeLog entry; it should be smeting like:
> 
> 2002-12-31  J. Brobecker  <brobecker@gnat.com>
> 
> 	* mdebugread.c (parse_symbol): Count until the stEnd matching
> 	the structure name.

Sure!

-- 
Joel

Re: [RFA] Fix a memory corruption in mdebugread.c

[Daniel Jacobowitz]

On Wed, Jan 01, 2003 at 05:35:18PM +0400, Joel Brobecker wrote:
> Hi Daniel,
> 
> > So I assume the stProc is nested directly in the type, and we generate
> > a complaint() in the default below?
> 
> Right.
> 
> > It's worth bearing in mind that we don't actually support C++ in
> > mdebugread; debugging the C++ part won't work well at all.  We won't
> > recognize any member methods for instance.
> 
> This is indeed good to know, although I kind of guessed this after
> reading the code in mdebugread.c :).
> 
> > My concern from your description is that a constructor may have the
> > same name as the enclosing type.  Are these mangled names?  Qualified
> > names?  Base names, in which case the constructor is a problem?  You
> > should be able to figure this out from looking at a couple of the names
> > found by the check below.
> 
> The method names are mangled, same for the constructors and destructors.
> For instance, one of the constructors for class TC_rule was
> __ct__7TC_ruleXRC7TC_rule. The destructor was __dt__7TC_ruleXv
> (actually, I assumed these stProc entries are constructors and
> destructors, but this a guess based on the name). An example of stProc
> entry for a method: FI_write__7TC_ruleXP4FILE.
> 
> stMembers, on the other hand, are not mangled. But they don't come with
> stEnd entries, so we should be safe.

OK.  Assuming that the name of the class's stEnd is not mangled like a
constructor, my worry is unfounded.  If you add a comment to that effect
then this patch is OK.

> 
> > That's not a proper ChangeLog entry; it should be smeting like:
> > 
> > 2002-12-31  J. Brobecker  <brobecker@gnat.com>
> > 
> > 	* mdebugread.c (parse_symbol): Count until the stEnd matching
> > 	the structure name.
> 
> Sure!
> 
> -- 
> Joel
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix a memory corruption in mdebugread.c

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

> OK.  Assuming that the name of the class's stEnd is not mangled like a
> constructor, my worry is unfounded.  If you add a comment to that effect
> then this patch is OK.

Class names appear not to be mangled, so indeed there should be no
name "clash". I checked the following change in (I expanded the comment
to explain that a method can not have the same name as the class name):

2003-01-03  J. Brobecker  <brobecker@gnat.com>

        * mdebugread.c (parse_symbol): Count until the stEnd matching
        the structure name.

-- 
Joel

[-- Attachment #2: mdebugread.c.diff --]
[-- Type: text/plain, Size: 1891 bytes --]

Index: mdebugread.c
===================================================================
RCS file: /cvs/src/src/gdb/mdebugread.c,v
retrieving revision 1.32
diff -c -3 -p -r1.32 mdebugread.c
*** mdebugread.c	17 Dec 2002 00:39:07 -0000	1.32
--- mdebugread.c	3 Jan 2003 13:59:00 -0000
*************** parse_symbol (SYMR *sh, union aux_ext *a
*** 865,871 ****
  	    switch (tsym.st)
  	      {
  	      case stEnd:
! 		goto end_of_fields;
  
  	      case stMember:
  		if (nfields == 0 && type_code == TYPE_CODE_UNDEF)
--- 865,888 ----
  	    switch (tsym.st)
  	      {
  	      case stEnd:
!                 /* C++ encodes class types as structures where there the
!                    methods are encoded as stProc. The scope of stProc
!                    symbols also ends with stEnd, thus creating a risk of
!                    taking the wrong stEnd symbol record as the end of
!                    the current struct, which would cause GDB to undercount
!                    the real number of fields in this struct.  To make sure
!                    we really reached the right stEnd symbol record, we
!                    check the associated name, and match it against the
!                    struct name.  Since method names are mangled while
!                    the class name is not, there is no risk of having a
!                    method whose name is identical to the class name
!                    (in particular constructor method names are different
!                    from the class name).  There is therefore no risk that
!                    this check stops the count on the StEnd of a method.  */
!                 if (strcmp (debug_info->ss + cur_fdr->issBase + tsym.iss,
!                             name) == 0)
!                   goto end_of_fields;
!                 break;
  
  	      case stMember:
  		if (nfields == 0 && type_code == TYPE_CODE_UNDEF)