[binutils-gdb] gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC

[Andrew Burgess <aburgess at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2fabdf3381dfc4bc89f6339c3db3e4bfe7ca8b14

commit 2fabdf3381dfc4bc89f6339c3db3e4bfe7ca8b14
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Sep 10 13:50:34 2018 +0100

    gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC
    
    This patch started as an observation from valgrind that GDB appeared
    to be loosing track of some memory associated with types.  An example
    valgrind stack would be:
    
      24 bytes in 1 blocks are possibly lost in loss record 419 of 5,361
         at 0x4C2EA1E: calloc (vg_replace_malloc.c:711)
         by 0x623D26: xcalloc (common-utils.c:85)
         by 0x623D65: xzalloc(unsigned long) (common-utils.c:95)
         by 0x72A066: make_function_type(type*, type**) (gdbtypes.c:510)
         by 0x72A098: lookup_function_type(type*) (gdbtypes.c:521)
         by 0x73635D: gdbtypes_post_init(gdbarch*) (gdbtypes.c:5439)
         by 0x727590: gdbarch_data(gdbarch*, gdbarch_data*) (gdbarch.c:5230)
         by 0x735B99: builtin_type(gdbarch*) (gdbtypes.c:5313)
         by 0x514D95: elf_rel_plt_read(minimal_symbol_reader&, objfile*, bfd_symbol**) (elfread.c:542)
         by 0x51662F: elf_read_minimal_symbols(objfile*, int, elfinfo const*) (elfread.c:1121)
         by 0x5168A5: elf_symfile_read(objfile*, enum_flags<symfile_add_flag>) (elfread.c:1207)
         by 0x8520F5: read_symbols(objfile*, enum_flags<symfile_add_flag>) (symfile.c:794)
    
    When we look in make_function_type we find a call to TYPE_ZALLOC
    (inside the INIT_FUNC_SPECIFIC macro).  It is this call to TYPE_ZALLOC
    that is allocating memory with xcalloc, that is then getting lost.
    
    The problem is tht calling TYPE_ALLOC or TYPE_ZALLOC currently
    allocates memory from either the objfile obstack or by using malloc.
    The problem with this is that types are allocated either on the
    objfile obstack, or on the gdbarch obstack.
    
    As a result, if we discard a type associated with an objfile then
    auxiliary data allocated with TYPE_(Z)ALLOC will be correctly
    discarded.  But, if we were ever to discard a gdbarch then any
    auxiliary type data would be leaked.  Right now there are very few
    places in GDB where a gdbarch is ever discarded, but it shouldn't hurt
    to close down these bugs as we spot them.
    
    This commit ensures that auxiliary type data is allocated from the
    same obstack as the type itself, which should reduce leaked memory.
    
    The one problem case that I found with this change was in eval.c,
    where in one place we allocate a local type structure, and then used
    TYPE_ZALLOC to allocate some space for the type.  This local type is
    neither object file owned, nor gdbarch owned, and so the updated
    TYPE_ALLOC code is unable to find an objstack to allocate space on.
    
    My proposed solution for this issue is that the space should be
    allocated with a direct call to xzalloc.  We could extend TYPE_ALLOC
    to check for type->gdbarch being null, and then fall back to a direct
    call to xzalloc, however, I think that making this rare case of a
    local type require special handling is not a bad thing, this serves to
    highlight that clearing up the memory will require special handling
    too.
    
    This special case of a local type is interesting as the types owner
    field (contained within the main_type) is completely null.  While
    reflecting on this I looked at how types use the get_type_arch
    function.  It seems clear that, based on how this is used, it is never
    intended that null will be returned from this function.  This only
    goes to reinforce, how locally alloctaed types, with no owner, are
    both special, and need to be handled carefully.  To help spot errors
    earlier, I added an assert into get_type_arch that the returned arch
    is not null.
    
    Inside gdbarch.c I found a few other places where auxiliary type data
    was being allocated directly on the heap rather than on the types
    obstack.  I have fixed these to call TYPE_ALLOC now.
    
    Finally, it is worth noting that as we don't clean up our gdbarch
    objects yet, then this will not make much of an impact on the amount
    of memory reported as lost at program termination time.  Memory
    allocated for auxiliary type information is still not freed, however,
    it is now on the correct obstack.  If we do ever start freeing our
    gdbarch structures then the associated type data will be cleaned up
    correctly.
    
    Tested on X86-64 GNU/Linux with no regressions.
    
    gdb/ChangeLog:
    
    	* eval.c (fake_method::fake_method): Call xzalloc directly for a
    	type that is neither object file owned, nor gdbarch owned.
    	* gdbtypes.c (get_type_gdbarch): Add an assert that returned
    	gdbarch is non-NULL.
    	(alloc_type_instance): Allocate non-objfile owned types on the
    	gdbarch obstack.
    	(copy_type_recursive): Allocate TYPE_FIELDS and TYPE_RANGE_DATA
    	using TYPE_ALLOC to ensure memory is allocated on the correct
    	obstack.
    	* gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile
    	obstack, or the gdbarch obstack.
    	(TYPE_ZALLOC): Rewrite using TYPE_ALLOC.

Diff:
---
 gdb/ChangeLog  | 15 +++++++++++++++
 gdb/eval.c     |  6 +++++-
 gdb/gdbtypes.c | 21 ++++++++++++++++-----
 gdb/gdbtypes.h | 37 +++++++++++++++++++++----------------
 4 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 49bcd9f..4e667b7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@
+2018-09-10  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* eval.c (fake_method::fake_method): Call xzalloc directly for a
+	type that is neither object file owned, nor gdbarch owned.
+	* gdbtypes.c (get_type_gdbarch): Add an assert that returned
+	gdbarch is non-NULL.
+	(alloc_type_instance): Allocate non-objfile owned types on the
+	gdbarch obstack.
+	(copy_type_recursive): Allocate TYPE_FIELDS and TYPE_RANGE_DATA
+	using TYPE_ALLOC to ensure memory is allocated on the correct
+	obstack.
+	* gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile
+	obstack, or the gdbarch obstack.
+	(TYPE_ZALLOC): Rewrite using TYPE_ALLOC.
+
 2018-09-14  Tom Tromey  <tom@tromey.com>
 
 	* infcall.c (call_function_by_hand_dummy): Remove unnecessary
diff --git a/gdb/eval.c b/gdb/eval.c
index 2e08e93..f9349e6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -683,9 +683,13 @@ fake_method::fake_method (type_instance_flags flags,
 	}
     }
 
+  /* We don't use TYPE_ZALLOC here to allocate space as TYPE is owned by
+     neither an objfile nor a gdbarch.  As a result we must manually
+     allocate memory for auxiliary fields, and free the memory ourselves
+     when we are done with it.  */
   TYPE_NFIELDS (type) = num_types;
   TYPE_FIELDS (type) = (struct field *)
-    TYPE_ZALLOC (type, sizeof (struct field) * num_types);
+    xzalloc (sizeof (struct field) * num_types);
 
   while (num_types-- > 0)
     TYPE_FIELD_TYPE (type, num_types) = param_types[num_types];
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 05bf7b1..9e87b8f 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -233,10 +233,19 @@ alloc_type_copy (const struct type *type)
 struct gdbarch *
 get_type_arch (const struct type *type)
 {
+  struct gdbarch *arch;
+
   if (TYPE_OBJFILE_OWNED (type))
-    return get_objfile_arch (TYPE_OWNER (type).objfile);
+    arch = get_objfile_arch (TYPE_OWNER (type).objfile);
   else
-    return TYPE_OWNER (type).gdbarch;
+    arch = TYPE_OWNER (type).gdbarch;
+
+  /* The ARCH can be NULL if TYPE is associated with neither an objfile nor
+     a gdbarch, however, this is very rare, and even then, in most cases
+     that get_type_arch is called, we assume that a non-NULL value is
+     returned.  */
+  gdb_assert (arch != NULL);
+  return arch;
 }
 
 /* See gdbtypes.h.  */
@@ -277,7 +286,7 @@ alloc_type_instance (struct type *oldtype)
   /* Allocate the structure.  */
 
   if (! TYPE_OBJFILE_OWNED (oldtype))
-    type = XCNEW (struct type);
+    type = GDBARCH_OBSTACK_ZALLOC (get_type_arch (oldtype), struct type);
   else
     type = OBSTACK_ZALLOC (&TYPE_OBJFILE (oldtype)->objfile_obstack,
 			   struct type);
@@ -4903,7 +4912,8 @@ copy_type_recursive (struct objfile *objfile,
       int i, nfields;
 
       nfields = TYPE_NFIELDS (type);
-      TYPE_FIELDS (new_type) = XCNEWVEC (struct field, nfields);
+      TYPE_FIELDS (new_type) = (struct field *)
+        TYPE_ZALLOC (new_type, nfields * sizeof (struct field));
       for (i = 0; i < nfields; i++)
 	{
 	  TYPE_FIELD_ARTIFICIAL (new_type, i) = 
@@ -4946,7 +4956,8 @@ copy_type_recursive (struct objfile *objfile,
   /* For range types, copy the bounds information.  */
   if (TYPE_CODE (type) == TYPE_CODE_RANGE)
     {
-      TYPE_RANGE_DATA (new_type) = XNEW (struct range_bounds);
+      TYPE_RANGE_DATA (new_type) = (struct range_bounds *)
+        TYPE_ALLOC (new_type, sizeof (struct range_bounds));
       *TYPE_RANGE_DATA (new_type) = *TYPE_RANGE_DATA (type);
     }
 
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eb7c365..32b58dc 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -49,6 +49,7 @@
 #include "common/enum-flags.h"
 #include "common/underlying.h"
 #include "common/print-utils.h"
+#include "gdbarch.h"
 
 /* Forward declarations for prototypes.  */
 struct field;
@@ -1717,26 +1718,30 @@ extern const struct floatformat *floatformats_vax_d[BFD_ENDIAN_UNKNOWN];
 extern const struct floatformat *floatformats_ibm_long_double[BFD_ENDIAN_UNKNOWN];
 
 
-/* * Allocate space for storing data associated with a particular
+/* Allocate space for storing data associated with a particular
    type.  We ensure that the space is allocated using the same
    mechanism that was used to allocate the space for the type
    structure itself.  I.e.  if the type is on an objfile's
    objfile_obstack, then the space for data associated with that type
-   will also be allocated on the objfile_obstack.  If the type is not
-   associated with any particular objfile (such as builtin types),
-   then the data space will be allocated with xmalloc, the same as for
-   the type structure.  */
-
-#define TYPE_ALLOC(t,size)  \
-   (TYPE_OBJFILE_OWNED (t) \
-    ? obstack_alloc (&TYPE_OBJFILE (t) -> objfile_obstack, size) \
-    : xmalloc (size))
-
-#define TYPE_ZALLOC(t,size)  \
-   (TYPE_OBJFILE_OWNED (t) \
-    ? memset (obstack_alloc (&TYPE_OBJFILE (t)->objfile_obstack, size),  \
-	      0, size)  \
-    : xzalloc (size))
+   will also be allocated on the objfile_obstack.  If the type is
+   associated with a gdbarch, then the space for data associated with that
+   type will also be allocated on the gdbarch_obstack.
+
+   If a type is not associated with neither an objfile or a gdbarch then
+   you should not use this macro to allocate space for data, instead you
+   should call xmalloc directly, and ensure the memory is correctly freed
+   when it is no longer needed.  */
+
+#define TYPE_ALLOC(t,size)                                              \
+  (obstack_alloc ((TYPE_OBJFILE_OWNED (t)                               \
+                   ? &TYPE_OBJFILE (t)->objfile_obstack                 \
+                   : gdbarch_obstack (TYPE_OWNER (t).gdbarch)),         \
+                  size))
+
+
+/* See comment on TYPE_ALLOC.  */
+
+#define TYPE_ZALLOC(t,size) (memset (TYPE_ALLOC (t, size), 0, size))
 
 /* Use alloc_type to allocate a type owned by an objfile.  Use
    alloc_type_arch to allocate a type owned by an architecture.  Use