pending/1008: [RFA] Fix a memory corruption in mdebugread.c

[Joel Brobecker <brobecker at gnat dot com>]

>Number:         1008
>Category:       pending
>Synopsis:       [RFA] Fix a memory corruption in mdebugread.c
>Confidential:   yes
>Severity:       serious
>Priority:       medium
>Responsible:    unassigned
>State:          open
>Class:          change-request
>Submitter-Id:   unknown
>Arrival-Date:   Fri Jan 31 06:08:00 UTC 2003
>Closed-Date:
>Last-Modified:
>Originator:     
>Release:        
>Organization:
>Environment:
>Description:
 --OgqxwSJOaUobr8KG
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 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
 
 --OgqxwSJOaUobr8KG
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="mdebugread.c.diff"
 
 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)
 
 --OgqxwSJOaUobr8KG--
 
>How-To-Repeat:
>Fix:
>Release-Note:
>Audit-Trail:
>Unformatted: