Fix value chain use-after-free

Hannes filed a bug showing a crash, where a pretty-printer written in
Python could cause a use-after-free.  He sent a patch, but I thought a
different approach was needed.

In a much earlier patch (see bug #12533), we changed the Python code
to release new values from the value chain when constructing a
gdb.Value.  The rationale for this is that if you write a command that
does a lot of computations in a loop, all the values will be kept live
by the value chain, resulting in gdb using a large amount of memory.

However, suppose a value is passed to Python from some code in gdb
that needs to use the value after the call into Python.  In this
scenario, value_to_value_object will still release the value -- and
because gdb code doesn't generally keep strong references to values (a
consequence of the ancient decision to use the value chain to avoid
memory management), this will result in a use-after-free.

This scenario can happen, as it turns out, when a value is passed to
Python for pretty-printing.  Now, normally this route boxes the value
via value_to_value_object_no_release, avoiding the problematic release
from the value chain.  However, if you then call Value.cast, the
underlying value API might return the same value, when is then
released from the chain.

This patch fixes the problem by changing how value boxing is done.
value_to_value_object no longer removes a value from the chain.
Instead, every spot in gdb that might construct new values uses a
scoped_value_mark to ensure that the requirements of bug #12533 are
met.  And, because incoming values aren't ever released from the chain
(the Value.cast one comes earlier on the chain than the
scoped_value_mark), the bug can no longer occur.  (Note that many
spots in the Python layer already take this approach, so not many
places needed to be touched.)

In the future I think we should replace the use of raw "value *" with
value_ref_ptr pretty much everywhere.  This will ensure lifetime
safety throughout gdb.

The test case in this patch comes from Hannes' original patch.  I only
made a trivial ("require") change to it.  However, while this fails
for him, I can't make it fail on this machine; nevertheless, he tried
my patch and reported the bug as being fixed.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30044

11 files changed, 70 insertions, 54 deletions

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index d4d1291..3b682f5 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -112,6 +112,8 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
 
   try
     {
+      scoped_value_mark free_values;
+
       struct symbol *func_symbol =
 	symbol_object_to_symbol (self_finishbp->func_symbol);
       struct value *function =
@@ -258,6 +260,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	      /* Remember only non-void return types.  */
 	      if (ret_type->code () != TYPE_CODE_VOID)
 		{
+		  scoped_value_mark free_values;
+
 		  /* Ignore Python errors at this stage.  */
 		  value *func_value = read_var_value (function, NULL, frame);
 		  self_bpfinish->function_value
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index f66d22b..ecd55d2 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -241,12 +241,13 @@ static PyObject *
 frapy_read_register (PyObject *self, PyObject *args)
 {
   PyObject *pyo_reg_id;
-  struct value *val = NULL;
+  PyObject *result = nullptr;
 
   if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
     return NULL;
   try
     {
+      scoped_value_mark free_values;
       frame_info_ptr frame;
       int regnum;
 
@@ -257,17 +258,19 @@ frapy_read_register (PyObject *self, PyObject *args)
 	return nullptr;
 
       gdb_assert (regnum >= 0);
-      val = value_of_register (regnum, frame);
+      struct value *val = value_of_register (regnum, frame);
 
       if (val == NULL)
 	PyErr_SetString (PyExc_ValueError, _("Can't read register."));
+      else
+	result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return val == NULL ? NULL : value_to_value_object (val);
+  return result;
 }
 
 /* Implementation of gdb.Frame.block (self) -> gdb.Block.
@@ -482,7 +485,6 @@ frapy_read_var (PyObject *self, PyObject *args)
   PyObject *sym_obj, *block_obj = NULL;
   struct symbol *var = NULL;	/* gcc-4.3.2 false warning.  */
   const struct block *block = NULL;
-  struct value *val = NULL;
 
   if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj))
     return NULL;
@@ -540,18 +542,21 @@ frapy_read_var (PyObject *self, PyObject *args)
       return NULL;
     }
 
+  PyObject *result = nullptr;
   try
     {
       FRAPY_REQUIRE_VALID (self, frame);
 
-      val = read_var_value (var, block, frame);
+      scoped_value_mark free_values;
+      struct value *val = read_var_value (var, block, frame);
+      result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (val);
+  return result;
 }
 
 /* Select this frame.  */
diff --git a/gdb/python/py-lazy-string.c b/gdb/python/py-lazy-string.c
index 7177d74..9b12054 100644
--- a/gdb/python/py-lazy-string.c
+++ b/gdb/python/py-lazy-string.c
@@ -104,7 +104,6 @@ static PyObject *
 stpy_convert_to_value (PyObject *self, PyObject *args)
 {
   lazy_string_object *self_string = (lazy_string_object *) self;
-  struct value *val = NULL;
 
   if (self_string->address == 0)
     {
@@ -113,10 +112,14 @@ stpy_convert_to_value (PyObject *self, PyObject *args)
       return NULL;
     }
 
+  PyObject *result = nullptr;
   try
     {
+      scoped_value_mark free_values;
+
       struct type *type = type_object_to_type (self_string->type);
       struct type *realtype;
+      struct value *val;
 
       gdb_assert (type != NULL);
       realtype = check_typedef (type);
@@ -141,13 +144,15 @@ stpy_convert_to_value (PyObject *self, PyObject *args)
 	  val = value_at_lazy (type, self_string->address);
 	  break;
 	}
+
+      result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (val);
+  return result;
 }
 
 static void
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index dbacb3f..29ae020 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -590,7 +590,7 @@ gdbpy_apply_val_pretty_printer (const struct extension_language_defn *extlang,
 
   gdbpy_enter enter_py (gdbarch, language);
 
-  gdbpy_ref<> val_obj (value_to_value_object_no_release (value));
+  gdbpy_ref<> val_obj (value_to_value_object (value));
   if (val_obj == NULL)
     {
       print_stack_unless_memory_error (stream);
diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index b877796..e6497ed 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -267,7 +267,6 @@ sympy_value (PyObject *self, PyObject *args)
   struct symbol *symbol = NULL;
   frame_info_ptr frame_info = NULL;
   PyObject *frame_obj = NULL;
-  struct value *value = NULL;
 
   if (!PyArg_ParseTuple (args, "|O", &frame_obj))
     return NULL;
@@ -285,6 +284,7 @@ sympy_value (PyObject *self, PyObject *args)
       return NULL;
     }
 
+  PyObject *result = nullptr;
   try
     {
       if (frame_obj != NULL)
@@ -301,14 +301,16 @@ sympy_value (PyObject *self, PyObject *args)
 	 was found, so we have no block to pass to read_var_value.  This will
 	 yield an incorrect value when symbol is not local to FRAME_INFO (this
 	 can happen with nested functions).  */
-      value = read_var_value (symbol, NULL, frame_info);
+      scoped_value_mark free_values;
+      struct value *value = read_var_value (symbol, NULL, frame_info);
+      result = value_to_value_object (value);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (value);
+  return result;
 }
 
 /* Given a symbol, and a symbol_object that has previously been
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index 0a37878..b68ec8d 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -957,7 +957,6 @@ typy_template_argument (PyObject *self, PyObject *args)
   const struct block *block = NULL;
   PyObject *block_obj = NULL;
   struct symbol *sym;
-  struct value *val = NULL;
 
   if (! PyArg_ParseTuple (args, "i|O", &argno, &block_obj))
     return NULL;
@@ -1014,16 +1013,19 @@ typy_template_argument (PyObject *self, PyObject *args)
       return NULL;
     }
 
+  PyObject *result = nullptr;
   try
     {
-      val = value_of_variable (sym, block);
+      scoped_value_mark free_values;
+      struct value *val = value_of_variable (sym, block);
+      result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (val);
+  return result;
 }
 
 static PyObject *
@@ -1189,6 +1191,7 @@ typy_optimized_out (PyObject *self, PyObject *args)
 {
   struct type *type = ((type_object *) self)->type;
 
+  scoped_value_mark free_values;
   return value_to_value_object (value::allocate_optimized_out (type));
 }
 
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 5b512c9..ee776bf 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -365,7 +365,6 @@ static PyObject *
 pending_framepy_read_register (PyObject *self, PyObject *args)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
-  struct value *val = NULL;
   int regnum;
   PyObject *pyo_reg_id;
 
@@ -380,25 +379,31 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
   if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
     return nullptr;
 
+  PyObject *result = nullptr;
   try
     {
+      scoped_value_mark free_values;
+
       /* Fetch the value associated with a register, whether it's
 	 a real register or a so called "user" register, like "pc",
 	 which maps to a real register.  In the past,
 	 get_frame_register_value() was used here, which did not
 	 handle the user register case.  */
-      val = value_of_register (regnum, pending_frame->frame_info);
+      struct value *val = value_of_register (regnum,
+					     pending_frame->frame_info);
       if (val == NULL)
 	PyErr_Format (PyExc_ValueError,
 		      "Cannot read register %d from frame.",
 		      regnum);
+      else
+	result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return val == NULL ? NULL : value_to_value_object (val);
+  return result;
 }
 
 /* Implementation of
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index c61de57..9441a43 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -1559,18 +1559,20 @@ valpy_nonzero (PyObject *self)
 static PyObject *
 valpy_invert (PyObject *self)
 {
-  struct value *val = NULL;
+  PyObject *result = nullptr;
 
   try
     {
-      val = value_complement (((value_object *) self)->value);
+      scoped_value_mark free_values;
+      struct value *val = value_complement (((value_object *) self)->value);
+      result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (val);
+  return result;
 }
 
 /* Implements left shift for value objects.  */
@@ -1774,32 +1776,10 @@ valpy_float (PyObject *self)
   return PyFloat_FromDouble (d);
 }
 
-/* Returns an object for a value which is released from the all_values chain,
-   so its lifetime is not bound to the execution of a command.  */
-PyObject *
-value_to_value_object (struct value *val)
-{
-  value_object *val_obj;
-
-  val_obj = PyObject_New (value_object, &value_object_type);
-  if (val_obj != NULL)
-    {
-      val_obj->value = release_value (val).release ();
-      val_obj->next = nullptr;
-      val_obj->prev = nullptr;
-      val_obj->address = NULL;
-      val_obj->type = NULL;
-      val_obj->dynamic_type = NULL;
-      note_value (val_obj);
-    }
-
-  return (PyObject *) val_obj;
-}
-
-/* Returns an object for a value, but without releasing it from the
+/* Returns an object for a value, without releasing it from the
    all_values chain.  */
 PyObject *
-value_to_value_object_no_release (struct value *val)
+value_to_value_object (struct value *val)
 {
   value_object *val_obj;
 
@@ -1926,21 +1906,23 @@ PyObject *
 gdbpy_history (PyObject *self, PyObject *args)
 {
   int i;
-  struct value *res_val = NULL;	  /* Initialize to appease gcc warning.  */
 
   if (!PyArg_ParseTuple (args, "i", &i))
     return NULL;
 
+  PyObject *result = nullptr;
   try
     {
-      res_val = access_value_history (i);
+      scoped_value_mark free_values;
+      struct value *res_val = access_value_history (i);
+      result = value_to_value_object (res_val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (res_val);
+  return result;
 }
 
 /* Add a gdb.Value into GDB's history, and return (as an integer) the
@@ -1988,15 +1970,23 @@ gdbpy_convenience_variable (PyObject *self, PyObject *args)
   if (!PyArg_ParseTuple (args, "s", &varname))
     return NULL;
 
+  PyObject *result = nullptr;
+  bool found = false;
   try
     {
       struct internalvar *var = lookup_only_internalvar (varname);
 
       if (var != NULL)
 	{
+	  scoped_value_mark free_values;
 	  res_val = value_of_internalvar (gdbpy_enter::get_gdbarch (), var);
 	  if (res_val->type ()->code () == TYPE_CODE_VOID)
 	    res_val = NULL;
+	  else
+	    {
+	      found = true;
+	      result = value_to_value_object (res_val);
+	    }
 	}
     }
   catch (const gdb_exception &except)
@@ -2004,10 +1994,10 @@ gdbpy_convenience_variable (PyObject *self, PyObject *args)
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  if (res_val == NULL)
+  if (result == nullptr && !found)
     Py_RETURN_NONE;
 
-  return value_to_value_object (res_val);
+  return result;
 }
 
 /* Set the value of a convenience variable.  */
diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
index 2db6793..d9fa355 100644
--- a/gdb/python/py-xmethods.c
+++ b/gdb/python/py-xmethods.c
@@ -423,6 +423,7 @@ python_xmethod_worker::do_get_result_type (value *obj,
       return EXT_LANG_RC_OK;
     }
 
+  scoped_value_mark free_values;
   obj_type = check_typedef (obj->type ());
   this_type = check_typedef (type_object_to_type (m_this_type));
   if (obj_type->code () == TYPE_CODE_PTR)
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 8fb0979..55bbe78 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -437,7 +437,6 @@ PyObject *symbol_to_symbol_object (struct symbol *sym);
 PyObject *block_to_block_object (const struct block *block,
 				 struct objfile *objfile);
 PyObject *value_to_value_object (struct value *v);
-PyObject *value_to_value_object_no_release (struct value *v);
 PyObject *type_to_type_object (struct type *);
 PyObject *frame_info_to_frame_object (frame_info_ptr frame);
 PyObject *symtab_to_linetable_object (PyObject *symtab);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 1ed13f2..fb3d975 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -970,22 +970,24 @@ static PyObject *
 gdbpy_parse_and_eval (PyObject *self, PyObject *args)
 {
   const char *expr_str;
-  struct value *result = NULL;
 
   if (!PyArg_ParseTuple (args, "s", &expr_str))
     return NULL;
 
+  PyObject *result = nullptr;
   try
     {
       gdbpy_allow_threads allow_threads;
-      result = parse_and_eval (expr_str);
+      scoped_value_mark free_values;
+      struct value *val = parse_and_eval (expr_str);
+      result = value_to_value_object (val);
     }
   catch (const gdb_exception &except)
     {
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
-  return value_to_value_object (result);
+  return result;
 }
 
 /* Implementation of gdb.invalidate_cached_frames.  */