[RFA] Fix a memory corruption in mdebugread.c

[Joel Brobecker <brobecker@gnat.com>]

[-- 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)