diff mbox

gdb: Don't leak memory with TYPE_ALLOC / TYPE_ZALLOC

Message ID 20180911141147.14107-1-andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Sept. 11, 2018, 2:11 p.m. UTC
I ran into the following issue after runnig valgrind on GDB.

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.

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 making the rare case of a local type
requiring special handling is not a bad thing, this serves to
highlight that clearing up the memory will require special handling
too.

Tested on X86-64 GNU/Linux with no regressions.

gdb/ChangeLog:

	* gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile
	obstack, or the gdbarch obstack.
	(TYPE_ZALLOC): Rewrite using TYPE_ALLOC.
	* eval.c (fake_method::fake_method): Call xzalloc directly for a
	type that is neither object file owned, nor gdbarch owned.
---
 gdb/ChangeLog  |  8 ++++++++
 gdb/eval.c     |  2 +-
 gdb/gdbtypes.h | 36 ++++++++++++++++++++----------------
 3 files changed, 29 insertions(+), 17 deletions(-)

Comments

Tom Tromey Sept. 11, 2018, 8:38 p.m. UTC | #1
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I ran into the following issue after runnig valgrind on GDB.
Andrew> Calling TYPE_ALLOC or TYPE_ZALLOC currently allocates memory from
Andrew> either the objfile obstack or by using malloc.  The problem with this
Andrew> is that types are allocated either on the objfile obstack, or on the
Andrew> gdbarch obstack.

Thank you for the patch.  I found the analysis very clear, so thanks for
that too.

Andrew> This commit ensures that auxiliary type data is allocated from the
Andrew> same obstack as the type itself, which should reduce leaked memory.

I'm curious whether this is something you intend to do.  gdb has many
things that are allocated for the lifetime of the process...

Andrew> The one problem case that I found with this change was in eval.c,
Andrew> where in one place we allocate a local type structure, and then used
Andrew> TYPE_ZALLOC to allocate some space for the type.  This local type is
Andrew> neither object file owned, nor gdbarch owned, and so the updated
Andrew> TYPE_ALLOC code is unable to find an objstack to allocate space on.

Andrew> My proposed solution for this issue is that the space should be
Andrew> allocated with a direct call to xzalloc.  We could extend TYPE_ALLOC
Andrew> to check for type->gdbarch being null, and then fall back to a direct
Andrew> call to xzalloc, however, I think making the rare case of a local type
Andrew> requiring special handling is not a bad thing, this serves to
Andrew> highlight that clearing up the memory will require special handling
Andrew> too.

I went and refreshed my memory of this area.

My first thought was that this would be dangerous because -- surely --
there was code allocating a type on the heap and manually managing it.
I was remembering Python, but I think we didn't do this because we
bailed on the whole "type GC" idea.  So, there no problems that I could
find.

I suppose if the type from fake_method ever ends up somewhere it
shouldn't -- passed to something that uses TYPE_ALLOC -- gdb will crash.

I wonder if get_type_arch should assert that the result is not NULL.

Andrew> 	* gdbtypes.h (TYPE_ALLOC): Allocate space on either the objfile
Andrew> 	obstack, or the gdbarch obstack.
Andrew> 	(TYPE_ZALLOC): Rewrite using TYPE_ALLOC.
Andrew> 	* eval.c (fake_method::fake_method): Call xzalloc directly for a
Andrew> 	type that is neither object file owned, nor gdbarch owned.

Andrew> +/* See comment on TYPE_ALLOC.  */
Andrew> +
Andrew> +#define TYPE_ZALLOC(t,size) (memset (TYPE_ALLOC (t, size), 0, size))
 
Would you mind seeing what happens if you use obstack_zalloc here
instead?  It seems like that would give us some extra compile-time
safety.  If it fails, that would be interesting.


I think the eval.c change should have a comment explaining why
TYPE_ZALLOC is not used.

This is ok with that nit changed.  If obstack_zalloc works the I think
that variant would be better.

thanks,
Tom
diff mbox

Patch

diff --git a/gdb/eval.c b/gdb/eval.c
index 2e08e9355f5..baa98e17545 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -685,7 +685,7 @@  fake_method::fake_method (type_instance_flags flags,
 
   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.h b/gdb/gdbtypes.h
index eb7c365b71d..3778b46839c 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,29 @@  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