[8/9] Use htab_up in type copying

Message ID 20200718172915.6811-9-tom@tromey.com
State New
Headers show
Series
  • Use htab_up in more places
Related show

Commit Message

Tom Tromey July 18, 2020, 5:29 p.m.
This changes create_copied_types_hash to return an htab_up, then
modifies the callers to avoid explicit use of htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* value.c (preserve_values): Update.
	* python/py-type.c (save_objfile_types): Update.
	* guile/scm-type.c (save_objfile_types): Update.
	* gdbtypes.h (create_copied_types_hash): Return htab_up.
	* gdbtypes.c (create_copied_types_hash): Return htab_up.
	* compile/compile-object-run.c (compile_object_run): Update.
---
 gdb/ChangeLog                    |  9 +++++++++
 gdb/compile/compile-object-run.c |  6 ++----
 gdb/gdbtypes.c                   | 10 +++++-----
 gdb/gdbtypes.h                   |  2 +-
 gdb/guile/scm-type.c             |  7 ++-----
 gdb/python/py-type.c             | 10 ++++------
 gdb/value.c                      | 11 ++++-------
 7 files changed, 27 insertions(+), 28 deletions(-)

-- 
2.17.2

Comments

Andrew Burgess Sept. 18, 2020, 5:32 p.m. | #1
* Tom Tromey <tom@tromey.com> [2020-07-18 11:29:14 -0600]:

> This changes create_copied_types_hash to return an htab_up, then

> modifies the callers to avoid explicit use of htab_delete.

> 

> gdb/ChangeLog

> 2020-07-18  Tom Tromey  <tom@tromey.com>

> 

> 	* value.c (preserve_values): Update.

> 	* python/py-type.c (save_objfile_types): Update.

> 	* guile/scm-type.c (save_objfile_types): Update.

> 	* gdbtypes.h (create_copied_types_hash): Return htab_up.

> 	* gdbtypes.c (create_copied_types_hash): Return htab_up.

> 	* compile/compile-object-run.c (compile_object_run): Update.


This introduced a use after free error, which is addressed by the
patch below.

Thanks,
Andrew

---


From 1c878706d8752b1f1ff61a8a5270675f2b4d828a Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>

Date: Fri, 18 Sep 2020 18:23:06 +0100
Subject: [PATCH] gdb: Fix use after free bug in compile_object_run

In this commit:

  commit 6108fd1823f9cf036bbbe528ffcdf2fee489b40a
  Date:   Thu Sep 17 11:47:50 2020 -0600

      Use htab_up in type copying

A use after free bug was introduced.  In compile-object-run.c, in the
function compile_object_run, the code used to look like this:

    htab_t copied_types;

    /* .... snip .... */

    /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
    copied_types = create_copied_types_hash (objfile);
    func_type = copy_type_recursive (objfile, func_type, copied_types);
    htab_delete (copied_types);

    /* .... snip .... */

    call_function_by_hand_dummy (func_val, NULL, args,
                                 do_module_cleanup, data);

The copied_types table exists on the obstack of objfile, but is
deleted once the call to copy_type_recursive has been completed.

After the change the code now looks like this:

    /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
    htab_up copied_types = create_copied_types_hash (objfile);
    func_type = copy_type_recursive (objfile, func_type, copied_types.get ());

    /* .... snip .... */

    call_function_by_hand_dummy (func_val, NULL, args,
                                 do_module_cleanup, data);

The copied_types is now a unique_ptr and deleted automatically when it
goes out of scope.

The problem however is that objfile, and its included obstack, may be
deleted by the call to do_module_cleanup, which is called by
call_function_by_hand_dummy.

This means that in the new code the objfile, and its obstack, are
deleted before copied_types is deleted, and as copied_types is on the
objfiles obstack, we are now reading undefined memory.

The solution in this commit is to wrap the call to
create_copied_types_hash and copy_type_recursive into a new static
helper function.  The htab_up will then be deleted within the new
function's scope, before objfile is deleted.

gdb/ChangeLog:

	* compile/compile-object-run.c (create_copied_type_recursive): New
	function.
	(compile_object_run): Use new function.
---
 gdb/ChangeLog                    |  6 ++++++
 gdb/compile/compile-object-run.c | 17 ++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 533478a0fb4..985c6f363a3 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -105,6 +105,16 @@ do_module_cleanup (void *arg, int registers_valid)
   xfree (data);
 }
 
+/* Create a copy of FUNC_TYPE that is independent of OBJFILE.  */
+
+static type *
+create_copied_type_recursive (objfile *objfile, type *func_type)
+{
+  htab_up copied_types = create_copied_types_hash (objfile);
+  func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
+  return func_type;
+}
+
 /* Perform inferior call of MODULE.  This function may throw an error.
    This function may leave files referenced by MODULE on disk until
    the inferior call dummy frame is discarded.  This function may throw errors.
@@ -143,9 +153,10 @@ compile_object_run (struct compile_module *module)
       int current_arg = 0;
       struct value **vargs;
 
-      /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
-      htab_up copied_types = create_copied_types_hash (objfile);
-      func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
+      /* OBJFILE may disappear while FUNC_TYPE is still in use as a
+	 result of the call to DO_MODULE_CLEANUP below, so we need a copy
+	 that does not depend on the objfile in anyway.  */
+      func_type = create_copied_type_recursive (objfile, func_type);
 
       gdb_assert (func_type->code () == TYPE_CODE_FUNC);
       func_val = value_from_pointer (lookup_pointer_type (func_type),
-- 
2.25.4
Tom Tromey Sept. 18, 2020, 6:03 p.m. | #2
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:


Andrew> A use after free bug was introduced.  In compile-object-run.c, in the
Andrew> function compile_object_run, the code used to look like this:

Oops, sorry.

Andrew> 	* compile/compile-object-run.c (create_copied_type_recursive): New
Andrew> 	function.
Andrew> 	(compile_object_run): Use new function.

Looks good.

Tom
Andrew Burgess Sept. 18, 2020, 6:20 p.m. | #3
* Tom Tromey <tom@tromey.com> [2020-09-18 12:03:05 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

> 

> Andrew> A use after free bug was introduced.  In compile-object-run.c, in the

> Andrew> function compile_object_run, the code used to look like this:

> 

> Oops, sorry.

> 

> Andrew> 	* compile/compile-object-run.c (create_copied_type_recursive): New

> Andrew> 	function.

> Andrew> 	(compile_object_run): Use new function.

> 

> Looks good.


Thanks,

Pushed.

Andrew

Patch

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index a2f39900053..533478a0fb4 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -140,14 +140,12 @@  compile_object_run (struct compile_module *module)
   try
     {
       struct type *func_type = SYMBOL_TYPE (func_sym);
-      htab_t copied_types;
       int current_arg = 0;
       struct value **vargs;
 
       /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
-      copied_types = create_copied_types_hash (objfile);
-      func_type = copy_type_recursive (objfile, func_type, copied_types);
-      htab_delete (copied_types);
+      htab_up copied_types = create_copied_types_hash (objfile);
+      func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
 
       gdb_assert (func_type->code () == TYPE_CODE_FUNC);
       func_val = value_from_pointer (lookup_pointer_type (func_type),
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index e87648813ec..9c395846895 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5201,13 +5201,13 @@  type_pair_eq (const void *item_lhs, const void *item_rhs)
    types without duplicates.  We use OBJFILE's obstack, because
    OBJFILE is about to be deleted.  */
 
-htab_t
+htab_up
 create_copied_types_hash (struct objfile *objfile)
 {
-  return htab_create_alloc_ex (1, type_pair_hash, type_pair_eq,
-			       NULL, &objfile->objfile_obstack,
-			       hashtab_obstack_allocate,
-			       dummy_obstack_deallocate);
+  return htab_up (htab_create_alloc_ex (1, type_pair_hash, type_pair_eq,
+					NULL, &objfile->objfile_obstack,
+					hashtab_obstack_allocate,
+					dummy_obstack_deallocate));
 }
 
 /* Recursively copy (deep copy) a dynamic attribute list of a type.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eaa4cff608d..3a8b5e7ad80 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2379,7 +2379,7 @@  extern int class_or_union_p (const struct type *);
 
 extern void maintenance_print_type (const char *, int);
 
-extern htab_t create_copied_types_hash (struct objfile *objfile);
+extern htab_up create_copied_types_hash (struct objfile *objfile);
 
 extern struct type *copy_type_recursive (struct objfile *objfile,
 					 struct type *type,
diff --git a/gdb/guile/scm-type.c b/gdb/guile/scm-type.c
index 19b7996c946..8fc9629eb0d 100644
--- a/gdb/guile/scm-type.c
+++ b/gdb/guile/scm-type.c
@@ -387,20 +387,17 @@  static void
 save_objfile_types (struct objfile *objfile, void *datum)
 {
   htab_t htab = (htab_t) datum;
-  htab_t copied_types;
 
   if (!gdb_scheme_initialized)
     return;
 
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   if (htab != NULL)
     {
-      htab_traverse_noresize (htab, tyscm_copy_type_recursive, copied_types);
+      htab_traverse_noresize (htab, tyscm_copy_type_recursive, copied_types.get ());
       htab_delete (htab);
     }
-
-  htab_delete (copied_types);
 }
 
 /* Administrivia for field smobs.  */
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index d0dfb52811b..f37a7652f6d 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -1068,7 +1068,6 @@  static void
 save_objfile_types (struct objfile *objfile, void *datum)
 {
   type_object *obj = (type_object *) datum;
-  htab_t copied_types;
 
   if (!gdb_python_initialized)
     return;
@@ -1077,23 +1076,22 @@  save_objfile_types (struct objfile *objfile, void *datum)
      operating on.  */
   gdbpy_enter enter_py (objfile->arch (), current_language);
 
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   while (obj)
     {
       type_object *next = obj->next;
 
-      htab_empty (copied_types);
+      htab_empty (copied_types.get ());
 
-      obj->type = copy_type_recursive (objfile, obj->type, copied_types);
+      obj->type = copy_type_recursive (objfile, obj->type,
+				       copied_types.get ());
 
       obj->next = NULL;
       obj->prev = NULL;
 
       obj = next;
     }
-
-  htab_delete (copied_types);
 }
 
 static void
diff --git a/gdb/value.c b/gdb/value.c
index 3a5b02bcb46..a018443a2c5 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2516,22 +2516,19 @@  preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
 void
 preserve_values (struct objfile *objfile)
 {
-  htab_t copied_types;
   struct internalvar *var;
 
   /* Create the hash table.  We allocate on the objfile's obstack, since
      it is soon to be deleted.  */
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   for (const value_ref_ptr &item : value_history)
-    preserve_one_value (item.get (), objfile, copied_types);
+    preserve_one_value (item.get (), objfile, copied_types.get ());
 
   for (var = internalvars; var; var = var->next)
-    preserve_one_internalvar (var, objfile, copied_types);
+    preserve_one_internalvar (var, objfile, copied_types.get ());
 
-  preserve_ext_lang_values (objfile, copied_types);
-
-  htab_delete (copied_types);
+  preserve_ext_lang_values (objfile, copied_types.get ());
 }
 
 static void