[RFA] Fix leaks in ada catch command

Message ID 20190211053728.7668-1-philippe.waroquiers@skynet.be
State New
Headers show
Series
  • [RFA] Fix leaks in ada catch command
Related show

Commit Message

Philippe Waroquiers Feb. 11, 2019, 5:37 a.m.
Valgrind reports leaks such as the below in various gdb.ada tests:
==14141== 114 bytes in 4 blocks are definitely lost in loss record 1,054 of 3,424
==14141==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==14141==    by 0x405107: xmalloc (common-utils.c:44)
==14141==    by 0x7563F9: xstrdup (xstrdup.c:34)
==14141==    by 0x381B21: ada_exception_sal (ada-lang.c:13217)
==14141==    by 0x381B21: create_ada_exception_catchpoint(gdbarch*, ada_exception_catchpoint_kind, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, int, int) (ada-lang.c:13251)
==14141==    by 0x3820A8: catch_ada_exception_command(char const*, int, cmd_list_element*) (ada-lang.c:13285)
==14141==    by 0x3F4828: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:1892)
...

==14141== 114 bytes in 4 blocks are definitely lost in loss record 1,055 of 3,424
==14141==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==14141==    by 0x405107: xmalloc (common-utils.c:44)
==14141==    by 0x7563F9: xstrdup (xstrdup.c:34)
==14141==    by 0x3B82B3: set_breakpoint_location_function(bp_location*, int) (breakpoint.c:7156)
==14141==    by 0x3C112B: add_location_to_breakpoint(breakpoint*, symtab_and_line const*) (breakpoint.c:8609)
==14141==    by 0x3C127A: init_raw_breakpoint(breakpoint*, gdbarch*, symtab_and_line, bptype, breakpoint_ops const*) (breakpoint.c:7187)
==14141==    by 0x3C1B52: init_ada_exception_breakpoint(breakpoint*, gdbarch*, symtab_and_line, char const*, breakpoint_ops const*, int, int, int) (breakpoint.c:11262)
==14141==    by 0x381C2E: create_ada_exception_catchpoint(gdbarch*, ada_exception_catchpoint_kind, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, int, int) (ada-lang.c:13255)
...

Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal.
Fix the second leak by calling the base bp_location dtor.

Tested on debian/amd64, natively and under valgrind.

gdb/ChangeLog
2019-02-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* breakpoint.c (bp_location_dtor): Rename to base_bp_location_dtor,
	make non static, and update users.
	* breakpoint.h (base_bp_location_dtor): Declare.
	* ada-lang.c (ada_catchpoint_location_dtor): Call
	base_bp_location_ops.dtor.
	(create_ada_exception_catchpoint): Free addr_string.
---
 gdb/ada-lang.c   | 9 +++++++--
 gdb/breakpoint.c | 8 ++++----
 gdb/breakpoint.h | 4 ++++
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.20.1

Comments

Tom Tromey Feb. 12, 2019, 9:35 p.m. | #1
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:


Philippe> Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal.
Philippe> Fix the second leak by calling the base bp_location dtor.

Philippe> Tested on debian/amd64, natively and under valgrind.

Thanks.

Philippe> -  const char *addr_string = NULL;
Philippe> +  char *addr_string = NULL;
Philippe>    const struct breakpoint_ops *ops = NULL;
Philippe> -  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
Philippe> +  struct symtab_and_line sal = ada_exception_sal (ex_kind,
Philippe> +						  (const char **) &addr_string,
Philippe> +						  &ops);

This cast seems questionable to me -- and since ada_exception_sal is
only called from this one spot, it seems like it would be just as
simple, but also more clear and more obviously correct to simply change
the type of "addr_string" to "std::string *".  This would also avoid any
potential leak if find_function_start_sal can throw (I didn't check).

Philippe>  static void
Philippe> -bp_location_dtor (struct bp_location *self)
Philippe> +base_bp_location_dtor (struct bp_location *self)
Philippe>  {
Philippe>    xfree (self->function_name);
Philippe>  }

For this part, I think it's easy in this case to just further C++-ify
bp_location.  I've done this in the appended -- let me know what you
think.

Tom

commit 65a60067205f97696a91dd7f134013ffa112dc21
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Feb 12 14:28:07 2019 -0700

    C++-ify bp_location
    
    Philippe noticed a memory leak coming from ada_catchpoint_location --
    it was not freeing the "function_name" member from its base class.
    
    This patch fixes the problem by further C++-ifying bp_location.  In
    particular, bp_location_ops is now removed, and the "dtor" function
    pointer is replaced with an ordinary destructor.
    
    gdb/ChangeLog
    2019-02-12  Tom Tromey  <tom@tromey.com>
    
            * breakpoint.c (~bp_location): Rename from bp_location_dtor.
            (bp_location_ops): Remove.
            (base_breakpoint_allocate_location): Update.
            (free_bp_location): Update.
            * ada-lang.c (class ada_catchpoint_location)
            <ada_catchpoint_location>: Remove ops parameter.
            (ada_catchpoint_location_dtor): Remove.
            (ada_catchpoint_location_ops): Remove.
            (allocate_location_exception): Update.
            * breakpoint.h (struct bp_location_ops): Remove.
            (class bp_location) <bp_location>: Remove bp_location_ops
            parameter.
            <~bp_location>: Add destructor.
            <ops>: Remove.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 194ae465769..a773943b8e8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,20 @@
+2019-02-12  Tom Tromey  <tom@tromey.com>
+
+	* breakpoint.c (~bp_location): Rename from bp_location_dtor.
+	(bp_location_ops): Remove.
+	(base_breakpoint_allocate_location): Update.
+	(free_bp_location): Update.
+	* ada-lang.c (class ada_catchpoint_location)
+	<ada_catchpoint_location>: Remove ops parameter.
+	(ada_catchpoint_location_dtor): Remove.
+	(ada_catchpoint_location_ops): Remove.
+	(allocate_location_exception): Update.
+	* breakpoint.h (struct bp_location_ops): Remove.
+	(class bp_location) <bp_location>: Remove bp_location_ops
+	parameter.
+	<~bp_location>: Add destructor.
+	<ops>: Remove.
+
 2019-02-12  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* symtab.h (struct minimal_symbol data_p): New const method.
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a878d4d1af2..602facb35e7 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12396,8 +12396,8 @@ static std::string ada_exception_catchpoint_cond_string
 class ada_catchpoint_location : public bp_location
 {
 public:
-  ada_catchpoint_location (const bp_location_ops *ops, breakpoint *owner)
-    : bp_location (ops, owner)
+  ada_catchpoint_location (breakpoint *owner)
+    : bp_location (owner)
   {}
 
   /* The condition that checks whether the exception that was raised
@@ -12406,24 +12406,6 @@ public:
   expression_up excep_cond_expr;
 };
 
-/* Implement the DTOR method in the bp_location_ops structure for all
-   Ada exception catchpoint kinds.  */
-
-static void
-ada_catchpoint_location_dtor (struct bp_location *bl)
-{
-  struct ada_catchpoint_location *al = (struct ada_catchpoint_location *) bl;
-
-  al->excep_cond_expr.reset ();
-}
-
-/* The vtable to be used in Ada catchpoint locations.  */
-
-static const struct bp_location_ops ada_catchpoint_location_ops =
-{
-  ada_catchpoint_location_dtor
-};
-
 /* An instance of this type is used to represent an Ada catchpoint.  */
 
 struct ada_catchpoint : public breakpoint
@@ -12493,7 +12475,7 @@ static struct bp_location *
 allocate_location_exception (enum ada_exception_catchpoint_kind ex,
 			     struct breakpoint *self)
 {
-  return new ada_catchpoint_location (&ada_catchpoint_location_ops, self);
+  return new ada_catchpoint_location (self);
 }
 
 /* Implement the RE_SET method in the breakpoint_ops structure for all
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 67d83e6f465..9be99ff4efa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6958,13 +6958,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
     }
 }
 
-bp_location::bp_location (const bp_location_ops *ops, breakpoint *owner)
+bp_location::bp_location (breakpoint *owner)
 {
   bp_location *loc = this;
 
-  gdb_assert (ops != NULL);
-
-  loc->ops = ops;
   loc->owner = owner;
   loc->cond_bytecode = NULL;
   loc->shlib_disabled = 0;
@@ -7033,7 +7030,6 @@ allocate_bp_location (struct breakpoint *bpt)
 static void
 free_bp_location (struct bp_location *loc)
 {
-  loc->ops->dtor (loc);
   delete loc;
 }
 
@@ -12166,19 +12162,11 @@ say_where (struct breakpoint *b)
     }
 }
 
-/* Default bp_location_ops methods.  */
-
-static void
-bp_location_dtor (struct bp_location *self)
+bp_location::~bp_location ()
 {
-  xfree (self->function_name);
+  xfree (function_name);
 }
 
-static const struct bp_location_ops bp_location_ops =
-{
-  bp_location_dtor
-};
-
 /* Destructor for the breakpoint base class.  */
 
 breakpoint::~breakpoint ()
@@ -12191,7 +12179,7 @@ breakpoint::~breakpoint ()
 static struct bp_location *
 base_breakpoint_allocate_location (struct breakpoint *self)
 {
-  return new bp_location (&bp_location_ops, self);
+  return new bp_location (self);
 }
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8c8c66a8158..a91e3e334cf 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -301,31 +301,19 @@ enum bp_loc_type
   bp_loc_other			/* Miscellaneous...  */
 };
 
-/* This structure is a collection of function pointers that, if
-   available, will be called instead of performing the default action
-   for this bp_loc_type.  */
-
-struct bp_location_ops
-{
-  /* Destructor.  Releases everything from SELF (but not SELF
-     itself).  */
-  void (*dtor) (struct bp_location *self);
-};
-
 class bp_location
 {
 public:
   bp_location () = default;
 
-  bp_location (const bp_location_ops *ops, breakpoint *owner);
+  bp_location (breakpoint *owner);
+
+  virtual ~bp_location ();
 
   /* Chain pointer to the next breakpoint location for
      the same parent breakpoint.  */
   bp_location *next = NULL;
 
-  /* Methods associated with this location.  */
-  const bp_location_ops *ops = NULL;
-
   /* The reference count.  */
   int refc = 0;
Philippe Waroquiers Feb. 12, 2019, 9:55 p.m. | #2
On Tue, 2019-02-12 at 14:35 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

> 

> Philippe> Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal.

> Philippe> Fix the second leak by calling the base bp_location dtor.

> 

> Philippe> Tested on debian/amd64, natively and under valgrind.

> 

> Thanks.

> 

> Philippe> -  const char *addr_string = NULL;

> Philippe> +  char *addr_string = NULL;

> Philippe>    const struct breakpoint_ops *ops = NULL;

> Philippe> -  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);

> Philippe> +  struct symtab_and_line sal = ada_exception_sal (ex_kind,

> Philippe> +						  (const char **) &addr_string,

> Philippe> +						  &ops);

> 

> This cast seems questionable to me -- and since ada_exception_sal is

> only called from this one spot, it seems like it would be just as

> simple, but also more clear and more obviously correct to simply change

> the type of "addr_string" to "std::string *".  This would also avoid any

> potential leak if find_function_start_sal can throw (I didn't check).

> 

> Philippe>  static void

> Philippe> -bp_location_dtor (struct bp_location *self)

> Philippe> +base_bp_location_dtor (struct bp_location *self)

> Philippe>  {

> Philippe>    xfree (self->function_name);

> Philippe>  }

> 

> For this part, I think it's easy in this case to just further C++-ify

> bp_location.  I've done this in the appended -- let me know what you

> think.

> 

> Tom


Yes, your suggestions to use std::string * and the below patch seem
better to me.  So, I assume you will push fixes for both

Thanks
Philippe

 



> 

> commit 65a60067205f97696a91dd7f134013ffa112dc21

> Author: Tom Tromey <tromey@adacore.com>

> Date:   Tue Feb 12 14:28:07 2019 -0700

> 

>     C++-ify bp_location

>     

>     Philippe noticed a memory leak coming from ada_catchpoint_location --

>     it was not freeing the "function_name" member from its base class.

>     

>     This patch fixes the problem by further C++-ifying bp_location.  In

>     particular, bp_location_ops is now removed, and the "dtor" function

>     pointer is replaced with an ordinary destructor.

>     

>     gdb/ChangeLog

>     2019-02-12  Tom Tromey  <tom@tromey.com>

>     

>             * breakpoint.c (~bp_location): Rename from bp_location_dtor.

>             (bp_location_ops): Remove.

>             (base_breakpoint_allocate_location): Update.

>             (free_bp_location): Update.

>             * ada-lang.c (class ada_catchpoint_location)

>             <ada_catchpoint_location>: Remove ops parameter.

>             (ada_catchpoint_location_dtor): Remove.

>             (ada_catchpoint_location_ops): Remove.

>             (allocate_location_exception): Update.

>             * breakpoint.h (struct bp_location_ops): Remove.

>             (class bp_location) <bp_location>: Remove bp_location_ops

>             parameter.

>             <~bp_location>: Add destructor.

>             <ops>: Remove.

> 

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog

> index 194ae465769..a773943b8e8 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,20 @@

> +2019-02-12  Tom Tromey  <tom@tromey.com>

> +

> +	* breakpoint.c (~bp_location): Rename from bp_location_dtor.

> +	(bp_location_ops): Remove.

> +	(base_breakpoint_allocate_location): Update.

> +	(free_bp_location): Update.

> +	* ada-lang.c (class ada_catchpoint_location)

> +	<ada_catchpoint_location>: Remove ops parameter.

> +	(ada_catchpoint_location_dtor): Remove.

> +	(ada_catchpoint_location_ops): Remove.

> +	(allocate_location_exception): Update.

> +	* breakpoint.h (struct bp_location_ops): Remove.

> +	(class bp_location) <bp_location>: Remove bp_location_ops

> +	parameter.

> +	<~bp_location>: Add destructor.

> +	<ops>: Remove.

> +

>  2019-02-12  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

>  

>  	* symtab.h (struct minimal_symbol data_p): New const method.

> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c

> index a878d4d1af2..602facb35e7 100644

> --- a/gdb/ada-lang.c

> +++ b/gdb/ada-lang.c

> @@ -12396,8 +12396,8 @@ static std::string ada_exception_catchpoint_cond_string

>  class ada_catchpoint_location : public bp_location

>  {

>  public:

> -  ada_catchpoint_location (const bp_location_ops *ops, breakpoint *owner)

> -    : bp_location (ops, owner)

> +  ada_catchpoint_location (breakpoint *owner)

> +    : bp_location (owner)

>    {}

>  

>    /* The condition that checks whether the exception that was raised

> @@ -12406,24 +12406,6 @@ public:

>    expression_up excep_cond_expr;

>  };

>  

> -/* Implement the DTOR method in the bp_location_ops structure for all

> -   Ada exception catchpoint kinds.  */

> -

> -static void

> -ada_catchpoint_location_dtor (struct bp_location *bl)

> -{

> -  struct ada_catchpoint_location *al = (struct ada_catchpoint_location *) bl;

> -

> -  al->excep_cond_expr.reset ();

> -}

> -

> -/* The vtable to be used in Ada catchpoint locations.  */

> -

> -static const struct bp_location_ops ada_catchpoint_location_ops =

> -{

> -  ada_catchpoint_location_dtor

> -};

> -

>  /* An instance of this type is used to represent an Ada catchpoint.  */

>  

>  struct ada_catchpoint : public breakpoint

> @@ -12493,7 +12475,7 @@ static struct bp_location *

>  allocate_location_exception (enum ada_exception_catchpoint_kind ex,

>  			     struct breakpoint *self)

>  {

> -  return new ada_catchpoint_location (&ada_catchpoint_location_ops, self);

> +  return new ada_catchpoint_location (self);

>  }

>  

>  /* Implement the RE_SET method in the breakpoint_ops structure for all

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c

> index 67d83e6f465..9be99ff4efa 100644

> --- a/gdb/breakpoint.c

> +++ b/gdb/breakpoint.c

> @@ -6958,13 +6958,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,

>      }

>  }

>  

> -bp_location::bp_location (const bp_location_ops *ops, breakpoint *owner)

> +bp_location::bp_location (breakpoint *owner)

>  {

>    bp_location *loc = this;

>  

> -  gdb_assert (ops != NULL);

> -

> -  loc->ops = ops;

>    loc->owner = owner;

>    loc->cond_bytecode = NULL;

>    loc->shlib_disabled = 0;

> @@ -7033,7 +7030,6 @@ allocate_bp_location (struct breakpoint *bpt)

>  static void

>  free_bp_location (struct bp_location *loc)

>  {

> -  loc->ops->dtor (loc);

>    delete loc;

>  }

>  

> @@ -12166,19 +12162,11 @@ say_where (struct breakpoint *b)

>      }

>  }

>  

> -/* Default bp_location_ops methods.  */

> -

> -static void

> -bp_location_dtor (struct bp_location *self)

> +bp_location::~bp_location ()

>  {

> -  xfree (self->function_name);

> +  xfree (function_name);

>  }

>  

> -static const struct bp_location_ops bp_location_ops =

> -{

> -  bp_location_dtor

> -};

> -

>  /* Destructor for the breakpoint base class.  */

>  

>  breakpoint::~breakpoint ()

> @@ -12191,7 +12179,7 @@ breakpoint::~breakpoint ()

>  static struct bp_location *

>  base_breakpoint_allocate_location (struct breakpoint *self)

>  {

> -  return new bp_location (&bp_location_ops, self);

> +  return new bp_location (self);

>  }

>  

>  static void

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h

> index 8c8c66a8158..a91e3e334cf 100644

> --- a/gdb/breakpoint.h

> +++ b/gdb/breakpoint.h

> @@ -301,31 +301,19 @@ enum bp_loc_type

>    bp_loc_other			/* Miscellaneous...  */

>  };

>  

> -/* This structure is a collection of function pointers that, if

> -   available, will be called instead of performing the default action

> -   for this bp_loc_type.  */

> -

> -struct bp_location_ops

> -{

> -  /* Destructor.  Releases everything from SELF (but not SELF

> -     itself).  */

> -  void (*dtor) (struct bp_location *self);

> -};

> -

>  class bp_location

>  {

>  public:

>    bp_location () = default;

>  

> -  bp_location (const bp_location_ops *ops, breakpoint *owner);

> +  bp_location (breakpoint *owner);

> +

> +  virtual ~bp_location ();

>  

>    /* Chain pointer to the next breakpoint location for

>       the same parent breakpoint.  */

>    bp_location *next = NULL;

>  

> -  /* Methods associated with this location.  */

> -  const bp_location_ops *ops = NULL;

> -

>    /* The reference count.  */

>    int refc = 0;

>
Tom Tromey Feb. 13, 2019, 1:47 p.m. | #3
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:


Philippe> Yes, your suggestions to use std::string * and the below patch seem
Philippe> better to me.  So, I assume you will push fixes for both

I've sent the patches now, please take a look.

Tom

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index a878d4d1af..04911f41a1 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12415,6 +12415,8 @@  ada_catchpoint_location_dtor (struct bp_location *bl)
   struct ada_catchpoint_location *al = (struct ada_catchpoint_location *) bl;
 
   al->excep_cond_expr.reset ();
+
+  base_bp_location_ops.dtor (bl);
 }
 
 /* The vtable to be used in Ada catchpoint locations.  */
@@ -13246,13 +13248,16 @@  create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 int disabled,
 				 int from_tty)
 {
-  const char *addr_string = NULL;
+  char *addr_string = NULL;
   const struct breakpoint_ops *ops = NULL;
-  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
+  struct symtab_and_line sal = ada_exception_sal (ex_kind,
+						  (const char **) &addr_string,
+						  &ops);
 
   std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ());
   init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
 				 ops, tempflag, disabled, from_tty);
+  xfree (addr_string);
   c->excep_string = excep_string;
   create_excep_cond_exprs (c.get (), ex_kind);
   if (!cond_string.empty ())
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bd05707d48..1562d4d1dc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12168,14 +12168,14 @@  say_where (struct breakpoint *b)
 /* Default bp_location_ops methods.  */
 
 static void
-bp_location_dtor (struct bp_location *self)
+base_bp_location_dtor (struct bp_location *self)
 {
   xfree (self->function_name);
 }
 
-static const struct bp_location_ops bp_location_ops =
+const struct bp_location_ops base_bp_location_ops =
 {
-  bp_location_dtor
+  base_bp_location_dtor
 };
 
 /* Destructor for the breakpoint base class.  */
@@ -12190,7 +12190,7 @@  breakpoint::~breakpoint ()
 static struct bp_location *
 base_breakpoint_allocate_location (struct breakpoint *self)
 {
-  return new bp_location (&bp_location_ops, self);
+  return new bp_location (&base_bp_location_ops, self);
 }
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8c8c66a815..76bc129001 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -312,6 +312,10 @@  struct bp_location_ops
   void (*dtor) (struct bp_location *self);
 };
 
+/* Default bp_location_ops methods.  */
+
+extern const struct bp_location_ops base_bp_location_ops;
+
 class bp_location
 {
 public: