Re: [RFA/Ada] guard against a malloc failure

Re: [RFA/Ada] guard against a malloc failure

[Joel Brobecker]

> 2008-11-27  Jerome Guitton  <guitton@adacore.com>
> 
> 	* ada-lang.c (ada_template_to_fixed_record_type_1): Check size
>         of type to guard against a crash.

Jerome and I just discussed this patch today, and we think we may
have a better solution. Standby...

-- 
Joel

Re: [RFA/Ada] guard against a malloc failure

[Jerome Guitton]

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

Joel Brobecker (brobecker@adacore.com):

> > 2008-11-27  Jerome Guitton  <guitton@adacore.com>
> > 
> > 	* ada-lang.c (ada_template_to_fixed_record_type_1): Check size
> >         of type to guard against a crash.
> 
> Jerome and I just discussed this patch today, and we think we may
> have a better solution. Standby...

Right. It is possible to rewrite ada_template_to_fixed_record_type_1
to avoid the crash; the discriminant value is allocated using the type
being built; the crash may happen when a bogus dynamic field is
included to this partial type. The trick is to allocate the value
before any of these dynamic fields is added to the partial type.  When
we encounter a dynamic field, all the discriminant fields have already
been added, so it should be fine.

Here is the new patch, tested on linux. OK to apply?

2008-12-12  Jerome Guitton  <guitton@adacore.com>

	* ada-lang.c (ada_template_to_fixed_record_type_1): Allocate dval
	before a dynamic field is added. 


[-- Attachment #2: ada-lang.c.diff --]
[-- Type: text/x-diff, Size: 1883 bytes --]

--- ada-lang.c.prev	2008-12-12 12:49:31.000000000 +0100
+++ ada-lang.c	2008-12-12 12:54:17.000000000 +0100
@@ -6831,7 +6831,7 @@ ada_template_to_fixed_record_type_1 (str
                                      int keep_dynamic_fields)
 {
   struct value *mark = value_mark ();
-  struct value *dval;
+  struct value *dval = dval0;
   struct type *rtype;
   int nfields, bit_len;
   int variant_field;
@@ -6882,10 +6882,17 @@ ada_template_to_fixed_record_type_1 (str
         }
       else if (is_dynamic_field (type, f))
         {
-          if (dval0 == NULL)
+          /* If dval is NULL, build it using the record type that we are
+             initializing. This takes advantage of the fact that the
+             discrimant fields should appear before any dynamic field;
+             so, at this point, the discriminant fields have already been
+             added to rtype. This property also assure that this dval will
+             be valid for the rest of the computation, no need to re-allocate
+             a new one for every dynamic field. Finally, as the value is
+             allocated before any  dynamic field has been added to the type,
+             we do not have to check its size before the allocation.  */
+          if (dval == NULL)
             dval = value_from_contents_and_address (rtype, valaddr, address);
-          else
-            dval = dval0;
 
           /* Get the fixed type of the field. Note that, in this case, we
              do not want to get the real type out of the tag: if the current
@@ -6931,10 +6938,8 @@ ada_template_to_fixed_record_type_1 (str
 
       off = TYPE_FIELD_BITPOS (rtype, variant_field);
 
-      if (dval0 == NULL)
+      if (dval == NULL)
         dval = value_from_contents_and_address (rtype, valaddr, address);
-      else
-        dval = dval0;
 
       branch_type =
         to_fixed_variant_branch_type

Re: [RFA/Ada] guard against a malloc failure

[Jerome Guitton]

Jerome Guitton (guitton@adacore.com):

> 2008-12-12  Jerome Guitton  <guitton@adacore.com>
> 
> 	* ada-lang.c (ada_template_to_fixed_record_type_1): Allocate dval
> 	before a dynamic field is added. 

Not quite right actually. The assumption that all discriminants are
emitted before any dynamic field is wrong. Consider:

   type X (Dx : Integer) is tagged record
      Vx : String (1 .. Dx);
   end record;

   type Y (Dx : Integer; Dy : Integer) is new X (Dx => Dx) with record
      Vy : String (1 .. Dy);
   end record;

The layout of Y's fields in memory will be: Dx, Vx, Y, Vy.

Apparently the patch does not fail on this case, but I do not
understand why. This needs to be clarified. Standby...

Re: [RFA/Ada] guard against a malloc failure

[Jerome Guitton]

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


Jerome Guitton (guitton@adacore.com):

> First fix for the test failure mentioned in:
> http://sourceware.org/ml/gdb-patches/2008-11/msg00718.html
> 
> ada_template_to_fixed_record_type_1 builds a fixed-size record type
> from the run-time values of its discriminants. If the record contains
> dynamic field, and if its discriminants are not initialized, the type
> may end up to be unreasonably big and GDB may fail to allocate a value
> of this type. This patch adds a check for such a case.

Summary of this thread: I first submitted a patch to guard against the
malloc failure, and then I thought that there was a way to change the
algorithm in order to avoid this check_size guard. Unfortunately, it
appears that there are cases that my new algorithm that does not
handle. So back to the original patch.  The new call to check_size is
not such a big deal after all; the built type size is checked at the
end of the function anyway.

2008-02-04  Jerome Guitton  <guitton@adacore.com>

	* ada-lang.c (ada_template_to_fixed_record_type_1): Check size
	of type to guard against a crash.


OK to apply?


[-- Attachment #2: check_size.diff --]
[-- Type: text/x-diff, Size: 943 bytes --]

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.187
diff -u -p -r1.187 ada-lang.c
--- ada-lang.c	13 Jan 2009 10:34:30 -0000	1.187
+++ ada-lang.c	4 Feb 2009 16:08:15 -0000
@@ -6877,7 +6877,15 @@ ada_template_to_fixed_record_type_1 (str
       else if (is_dynamic_field (type, f))
         {
           if (dval0 == NULL)
-            dval = value_from_contents_and_address (rtype, valaddr, address);
+	    {
+	      /* rtype's length is computed based on the run-time
+		 value of discriminants.  If the discriminants are not
+		 initialized, the type size may be completely bogus and
+		 GDB may fail to allocate a value for it. So check the
+		 size first before creating the value.  */
+	      check_size (rtype);
+	      dval = value_from_contents_and_address (rtype, valaddr, address);
+	    }
           else
             dval = dval0;
 

Re: [RFA/Ada] guard against a malloc failure

[Joel Brobecker]

> 2008-02-04  Jerome Guitton  <guitton@adacore.com>
> 
> 	* ada-lang.c (ada_template_to_fixed_record_type_1): Check size
> 	of type to guard against a crash.
> 
> 
> OK to apply?

Yes, please do!

Thank you,
-- 
Joel

Re: [RFA/Ada] guard against a malloc failure

[Jerome Guitton]

Joel Brobecker (brobecker@adacore.com):

> > 2008-02-04  Jerome Guitton  <guitton@adacore.com>
> > 
> > 	* ada-lang.c (ada_template_to_fixed_record_type_1): Check size
> > 	of type to guard against a crash.
> > 
> > 
> > OK to apply?
> 
> Yes, please do!

Done! Thanks.