Re: [RFA] completer.c (expression_completer): Stop memory leak.

[Michael Snyder <msnyder@vmware.com>]

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

Pedro Alves wrote:
> On Wednesday 09 March 2011 18:58:30, Michael Snyder wrote:
>> In this case, it is possible for fieldname to be allocated before an 
>> exception is thrown.
>>
>> OK?
> 
> Notice how `fieldname' is uninitialized by expression_completer.
> If an exception is thrown from within parse_field_expression
> before writting to `fieldname', you'll be calling `free'
> (it should be xfree, btw) on an uninitialized pointer.  That's
> bad.
> 
> Please fix this within parse_field_expression itself.
> 1) even if what I describe above can't happen as is
> today (it may or not, dunno), your change makes the
> code quite fragile.  2) any other parse_field_expression call
> that isn't wrapped in a TRY_CATCH like this, is a
> potential leak.
> 

OK how is this?  I'm a little uncertain about the way
"name" is handled there at the end...



[-- Attachment #2: parse.txt --]
[-- Type: text/plain, Size: 1439 bytes --]

2011-03-09  Michael Snyder  <msnyder@vmware.com>

	* parse.c (parse_field_expression): Clean up memory gracefullly.

Index: parse.c
===================================================================
RCS file: /cvs/src/src/gdb/parse.c,v
retrieving revision 1.108
diff -u -p -r1.108 parse.c
--- parse.c	5 Mar 2011 22:02:47 -0000	1.108
+++ parse.c	9 Mar 2011 20:27:08 -0000
@@ -1211,34 +1211,37 @@ parse_field_expression (char *string, ch
   struct value *val;
   int subexp;
   volatile struct gdb_exception except;
+  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
 
   TRY_CATCH (except, RETURN_MASK_ERROR)
     {
       in_parse_field = 1;
       exp = parse_exp_in_context (&string, 0, 0, 0, &subexp);
+      make_cleanup (xfree, exp);
     }
   in_parse_field = 0;
   if (except.reason < 0 || ! exp)
     return NULL;
   if (expout_last_struct == -1)
     {
-      xfree (exp);
+      do_cleanups (cleanups);
       return NULL;
     }
 
   *name = extract_field_op (exp, &subexp);
   if (!*name)
     {
-      xfree (exp);
+      do_cleanups (cleanups);
       return NULL;
     }
+  make_cleanup (xfree, name);
 
   /* This might throw an exception.  If so, we want to let it
      propagate.  */
   val = evaluate_subexpression_type (exp, subexp);
   /* (*NAME) is a part of the EXP memory block freed below.  */
   *name = xstrdup (*name);
-  xfree (exp);
+  do_cleanups (cleanups);
 
   return value_type (val);
 }