gdb/varobj: Fix use after free in varobj

Varobj object contains references to types, variables (i.e. struct
variable) and expression.  All of those can reference data on an
objfile's obstack.  It is possible for this objfile to be deleted (and
the obstack to be feed), while the varobj remains valid.  Later, if the
user uses the varobj, this will result in a use-after-free error.  With
address sanitizer build, this leads to a plain error.  For non address
sanitizer build we might see undefined behaviour, which manifest
themself as assertion failures when accessing data backed by feed
memory.

This can be observed if we create a varobj that refers to ta symbol in a
shared library, after either the objfile gets reloaded (using the `file`
command) or after the shared library is unloaded (with a call to dlclose
for example).

This patch fixes those issues by:

- Adding cleanup procedure to the free_objfile observable.  When
  activated this observer clears expressions referencing the objfile
  being freed, and removes references to blocks belonging to this
  objfile.
- Adding varobj support in the `preserve_values` (gdb.value.c).  This
  ensures that before the objfile is unloaded, any type owned by the
  objfile referenced by the varobj is replaced by an equivalent type
  not owned by the objfile.  This process is done here instead of in the
  free_objfile observer in order to reuse the type hash table already
  used for similar purpose when replacing types of values kept in the
  value history.

This patch also makes sure to keep a reference to the expression's
gdbarch and language_defn members when the varobj->root->exp is
initialized.  Those structures outlive the objfile, so this is safe.
This is done because those references might be used initialize a python
context even after exp is invalidated.  Another approach could have been
to initialize the python context with default gdbarch and language_defn
(i.e. nullptr) if expr is NULL, but since we might still try to display
the value which was obtained by evaluating exp when it was still valid,
keeping track of the context which was used at this time seems
reasonable.

Tested on x86_64-Linux.

Co-Authored-By: Pedro Alves <pedro@palves.net>

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
new file mode 100644 (file)
index 0000000..3d4c7aa
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
@@ -0,0 +1,30 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct bar
+{
+  int a;
+  int b;
+};
+
+struct bar global_shlib_var = { 2, 2 };
+
+void
+foo ()
+{
+  struct bar local_var = { 1, 1 };
+}

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
new file mode 100644 (file)
index 0000000..f9237c3
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
@@ -0,0 +1,41 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <dlfcn.h>
+
+static void
+no_varobj_in_scope (void)
+{
+}
+
+static void
+floating_varobj_in_scope (void)
+{
+  int local_var = 0;
+}
+
+int
+main (int argc, char *argv [])
+{
+  void *shlib = dlopen (SHLIB_PATH, RTLD_LAZY);
+  void (*foo)() = dlsym (shlib, "foo");
+  foo ();
+  dlclose (shlib);
+
+  no_varobj_in_scope ();
+  floating_varobj_in_scope ();
+}

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
new file mode 100644 (file)
index 0000000..b405c83
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -0,0 +1,93 @@
+# Copyright 2007-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Test that varobj are invalidated after the shlib they point to goes
+# away.
+
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+if { [skip_shlib_tests] } {
+    return 0
+}
+
+standard_testfile .c -lib.c
+set shlib_path [standard_output_file ${testfile}-lib.so]
+
+if { [gdb_compile_shlib $srcdir/$subdir/$srcfile2 $shlib_path {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+
+set opts [list shlib_load debug additional_flags=-DSHLIB_PATH="${shlib_path}"]
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+proc do_test { separate_debuginfo } {
+    if { $separate_debuginfo } {
+       gdb_gnu_strip_debug $::shlib_path
+    }
+
+    if { [mi_clean_restart] } {
+       unsupported "failed to start GDB"
+       return
+    }
+
+    # Start the process once and create varobjs referencing the loaded objfiles.
+    with_test_prefix "setup" {
+       mi_load_shlibs $::shlib_path
+       if { $separate_debuginfo } {
+           mi_load_shlibs ${::shlib_path}.debug
+       }
+       mi_delete_breakpoints
+       mi_gdb_reinitialize_dir $::srcdir/$::subdir
+       mi_gdb_load $::binfile
+
+       mi_runto foo -pending
+
+       mi_create_varobj global_shlib_var global_shlib_var "create global gloal_shlib_var"
+       mi_create_floating_varobj floating_local local_var "create floating local_var"
+
+       # Advance to a point where the shlib's objfile have been deleted.
+       mi_continue_to "no_varobj_in_scope"
+    }
+
+    with_test_prefix "after objfile deleted" {
+       # The global shlib var was invalidated when the objfile got unloaded.
+       mi_gdb_test "-var-update global_shlib_var" \
+           "\\^done,changelist=\\\[\{name=\"global_shlib_var\",in_scope=\"invalid\",has_more=\"0\"\}\]" \
+           "global_shlib_var invalidated"
+
+       # The floating var is still valid but not in scope.
+       mi_gdb_test "-var-update floating_local" \
+           "\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"false\",type_changed=\"false\",has_more=\"0\"}\\\]" \
+           "floating_local still valid but not in scope"
+
+       # The varobj can be re-evaluated if the expression is valid in the current
+       # frame.
+       mi_continue_to "floating_varobj_in_scope"
+       mi_gdb_test "-var-update floating_local" \
+           "\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"true\",type_changed=\"true\",new_type=\"int\",new_num_children=\"0\",has_more=\"0\"}\\\]" \
+           "floating_local in scope with new type and value"
+    }
+}
+
+foreach_with_prefix separate_debuginfo {0 1} {
+    do_test $separate_debuginfo
+}

diff --git a/gdb/value.c b/gdb/value.c
index c9bec678d95758fb36448f7609c5860af8878ab2..d027eef07e577e675224dc9c11799631de231e02 100644 (file)
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -49,6 +49,7 @@
 #include "cli/cli-style.h"
 #include "expop.h"
 #include "inferior.h"
+#include "varobj.h"
 
 /* Definition of a user function.  */
 struct internal_function
@@ -2599,6 +2600,25 @@ preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
     }
 }
 
+/* Make sure that all types and values referenced by VAROBJ are updated before
+   OBJFILE is discarded.  COPIED_TYPES is used to prevent cycles and
+   duplicates.  */
+
+static void
+preserve_one_varobj (struct varobj *varobj, struct objfile *objfile,
+                    htab_t copied_types)
+{
+  if (varobj->type->is_objfile_owned ()
+      && varobj->type->objfile_owner () == objfile)
+    {
+      varobj->type
+       = copy_type_recursive (objfile, varobj->type, copied_types);
+    }
+
+  if (varobj->value != nullptr)
+    preserve_one_value (varobj->value.get (), objfile, copied_types);
+}
+
 /* Update the internal variables and value history when OBJFILE is
    discarded; we must copy the types out of the objfile.  New global types
    will be created for every convenience variable which currently points to
@@ -2620,6 +2640,13 @@ preserve_values (struct objfile *objfile)
   for (var = internalvars; var; var = var->next)
     preserve_one_internalvar (var, objfile, copied_types.get ());
 
+  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
+  all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
+    {
+      preserve_one_varobj (varobj, objfile,
+                          copied_types.get ());
+    });
+
   preserve_ext_lang_values (objfile, copied_types.get ());
 }
 

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 9a6d579ddf0c94cd204f396a646b67e7a7a02c1a..2b6dfff6888315da8ce8bfde1d6b0cd62370aadd 100644 (file)
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -32,6 +32,7 @@
 #include "parser-defs.h"
 #include "gdbarch.h"
 #include <algorithm>
+#include "observable.h"
 
 #if HAVE_PYTHON
 #include "python/python.h"
@@ -72,6 +73,12 @@ struct varobj_root
   /* The expression for this parent.  */
   expression_up exp;
 
+  /* Cached arch from exp, for use in case exp gets invalidated.  */
+  struct gdbarch *gdbarch = nullptr;
+
+  /* Cached language from exp, for use in case exp gets invalidated.  */
+  const struct language_defn *language_defn = nullptr;
+
   /* Block for which this expression is valid.  */
   const struct block *valid_block = NULL;
 
@@ -206,7 +213,7 @@ is_root_p (const struct varobj *var)
 
 /* See python-internal.h.  */
 gdbpy_enter_varobj::gdbpy_enter_varobj (const struct varobj *var)
-: gdbpy_enter (var->root->exp->gdbarch, var->root->exp->language_defn)
+: gdbpy_enter (var->root->gdbarch, var->root->language_defn)
 {
 }
 
@@ -303,6 +310,11 @@ varobj_create (const char *objname,
       try
        {
          var->root->exp = parse_exp_1 (&p, pc, block, 0, &tracker);
+
+         /* Cache gdbarch and language_defn as they might be used even
+            after var is invalidated and var->root->exp cleared.  */
+         var->root->gdbarch = var->root->exp->gdbarch;
+         var->root->language_defn = var->root->exp->language_defn;
        }
 
       catch (const gdb_exception_error &except)
@@ -2378,6 +2390,55 @@ varobj_invalidate (void)
   all_root_varobjs (varobj_invalidate_iter);
 }
 
+/* Ensure that no varobj keep references to OBJFILE.  */
+
+static void
+varobj_invalidate_if_uses_objfile (struct objfile *objfile)
+{
+  if (objfile->separate_debug_objfile_backlink != nullptr)
+    objfile = objfile->separate_debug_objfile_backlink;
+
+  all_root_varobjs ([objfile] (struct varobj *var)
+    {
+      if (var->root->valid_block != nullptr)
+       {
+         struct objfile *bl_objfile = block_objfile (var->root->valid_block);
+         if (bl_objfile->separate_debug_objfile_backlink != nullptr)
+           bl_objfile = bl_objfile->separate_debug_objfile_backlink;
+
+         if (bl_objfile == objfile)
+           {
+             /* The varobj is tied to a block which is going away.  There is
+                no way to reconstruct something later, so invalidate the
+                varobj completly and drop the reference to the block which is
+                being freed.  */
+             var->root->is_valid = false;
+             var->root->valid_block = nullptr;
+           }
+       }
+
+      if (var->root->exp != nullptr
+         && exp_uses_objfile (var->root->exp.get (), objfile))
+       {
+         /* The varobj's current expression references the objfile.  For
+            globals and floating, it is possible that when we try to
+            re-evaluate the expression later it is still valid with
+            whatever is in scope at that moment.  Just invalidate the
+            expression for now.  */
+         var->root->exp.reset ();
+
+         /* It only makes sense to keep a floating varobj around.  */
+         if (!var->root->floating)
+           var->root->is_valid = false;
+       }
+
+      /* var->value->type and var->type might also reference the objfile.
+        This is taken care of in value.c:preserve_values which deals with
+        making sure that objfile-owend types are replaced with
+        gdbarch-owned equivalents.  */
+    });
+}
+
 /* A hash function for a varobj.  */
 
 static hashval_t
@@ -2412,4 +2473,7 @@ _initialize_varobj ()
                             _("When non-zero, varobj debugging is enabled."),
                             NULL, show_varobjdebug,
                             &setdebuglist, &showdebuglist);
+
+  gdb::observers::free_objfile.attach (varobj_invalidate_if_uses_objfile,
+                                      "varobj");
 }