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

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

[Michael Snyder]

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

In this case, it is possible for fieldname to be allocated before an 
exception is thrown.

OK?


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

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

	* completer.c (expression_completer): Stop memory leak.

Index: completer.c
===================================================================
RCS file: /cvs/src/src/gdb/completer.c,v
retrieving revision 1.44
diff -u -p -r1.44 completer.c
--- completer.c	26 Feb 2011 02:07:07 -0000	1.44
+++ completer.c	9 Mar 2011 18:56:24 -0000
@@ -455,7 +455,10 @@ expression_completer (struct cmd_list_el
       type = parse_field_expression (text, &fieldname);
     }
   if (except.reason < 0)
-    return NULL;
+    {
+      free (fieldname);
+      return NULL;
+    }
   if (fieldname && type)
     {
       for (;;)

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

[Pedro Alves]

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.

-- 
Pedro Alves

> 
> completer.txt
>   2011-03-09  Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>
> 
>         * completer.c (expression_completer): Stop memory leak.
> 
> Index: completer.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/completer.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 completer.c
> --- completer.c 26 Feb 2011 02:07:07 -0000      1.44
> +++ completer.c 9 Mar 2011 18:56:24 -0000
> @@ -455,7 +455,10 @@ expression_completer (struct cmd_list_el
>        type = parse_field_expression (text, &fieldname);
>      }
>    if (except.reason < 0)
> -    return NULL;
> +    {
> +      free (fieldname);
> +      return NULL;

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

[Michael Snyder]

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

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

[Pedro Alves]

On Wednesday 09 March 2011 20:29:22, Michael Snyder wrote:

> OK how is this?  

Looks completely untested.  I'd be most surprised if it
didn't crash.  Please, please don't have me review untested
patches.

-- 
Pedro Alves