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
This commit is contained in:
Tom Tromey
2023-02-08 13:59:36 -07:00
parent 9b955acd7f
commit f3d3bbbcdd
14 changed files with 173 additions and 54 deletions

View File

@ -112,6 +112,8 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
try try
{ {
scoped_value_mark free_values;
struct symbol *func_symbol = struct symbol *func_symbol =
symbol_object_to_symbol (self_finishbp->func_symbol); symbol_object_to_symbol (self_finishbp->func_symbol);
struct value *function = struct value *function =
@ -258,6 +260,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
/* Remember only non-void return types. */ /* Remember only non-void return types. */
if (ret_type->code () != TYPE_CODE_VOID) if (ret_type->code () != TYPE_CODE_VOID)
{ {
scoped_value_mark free_values;
/* Ignore Python errors at this stage. */ /* Ignore Python errors at this stage. */
value *func_value = read_var_value (function, NULL, frame); value *func_value = read_var_value (function, NULL, frame);
self_bpfinish->function_value self_bpfinish->function_value

View File

@ -241,12 +241,13 @@ static PyObject *
frapy_read_register (PyObject *self, PyObject *args) frapy_read_register (PyObject *self, PyObject *args)
{ {
PyObject *pyo_reg_id; PyObject *pyo_reg_id;
struct value *val = NULL; PyObject *result = nullptr;
if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id)) if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
return NULL; return NULL;
try try
{ {
scoped_value_mark free_values;
frame_info_ptr frame; frame_info_ptr frame;
int regnum; int regnum;
@ -257,17 +258,19 @@ frapy_read_register (PyObject *self, PyObject *args)
return nullptr; return nullptr;
gdb_assert (regnum >= 0); gdb_assert (regnum >= 0);
val = value_of_register (regnum, frame); struct value *val = value_of_register (regnum, frame);
if (val == NULL) if (val == NULL)
PyErr_SetString (PyExc_ValueError, _("Can't read register.")); PyErr_SetString (PyExc_ValueError, _("Can't read register."));
else
result = value_to_value_object (val);
} }
catch (const gdb_exception &except) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_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. /* 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; PyObject *sym_obj, *block_obj = NULL;
struct symbol *var = NULL; /* gcc-4.3.2 false warning. */ struct symbol *var = NULL; /* gcc-4.3.2 false warning. */
const struct block *block = NULL; const struct block *block = NULL;
struct value *val = NULL;
if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj)) if (!PyArg_ParseTuple (args, "O|O", &sym_obj, &block_obj))
return NULL; return NULL;
@ -540,18 +542,21 @@ frapy_read_var (PyObject *self, PyObject *args)
return NULL; return NULL;
} }
PyObject *result = nullptr;
try try
{ {
FRAPY_REQUIRE_VALID (self, frame); 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return value_to_value_object (val); return result;
} }
/* Select this frame. */ /* Select this frame. */

View File

@ -104,7 +104,6 @@ static PyObject *
stpy_convert_to_value (PyObject *self, PyObject *args) stpy_convert_to_value (PyObject *self, PyObject *args)
{ {
lazy_string_object *self_string = (lazy_string_object *) self; lazy_string_object *self_string = (lazy_string_object *) self;
struct value *val = NULL;
if (self_string->address == 0) if (self_string->address == 0)
{ {
@ -113,10 +112,14 @@ stpy_convert_to_value (PyObject *self, PyObject *args)
return NULL; return NULL;
} }
PyObject *result = nullptr;
try try
{ {
scoped_value_mark free_values;
struct type *type = type_object_to_type (self_string->type); struct type *type = type_object_to_type (self_string->type);
struct type *realtype; struct type *realtype;
struct value *val;
gdb_assert (type != NULL); gdb_assert (type != NULL);
realtype = check_typedef (type); realtype = check_typedef (type);
@ -141,13 +144,15 @@ stpy_convert_to_value (PyObject *self, PyObject *args)
val = value_at_lazy (type, self_string->address); val = value_at_lazy (type, self_string->address);
break; break;
} }
result = value_to_value_object (val);
} }
catch (const gdb_exception &except) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return value_to_value_object (val); return result;
} }
static void static void

View File

@ -590,7 +590,7 @@ gdbpy_apply_val_pretty_printer (const struct extension_language_defn *extlang,
gdbpy_enter enter_py (gdbarch, language); 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) if (val_obj == NULL)
{ {
print_stack_unless_memory_error (stream); print_stack_unless_memory_error (stream);

View File

@ -267,7 +267,6 @@ sympy_value (PyObject *self, PyObject *args)
struct symbol *symbol = NULL; struct symbol *symbol = NULL;
frame_info_ptr frame_info = NULL; frame_info_ptr frame_info = NULL;
PyObject *frame_obj = NULL; PyObject *frame_obj = NULL;
struct value *value = NULL;
if (!PyArg_ParseTuple (args, "|O", &frame_obj)) if (!PyArg_ParseTuple (args, "|O", &frame_obj))
return NULL; return NULL;
@ -285,6 +284,7 @@ sympy_value (PyObject *self, PyObject *args)
return NULL; return NULL;
} }
PyObject *result = nullptr;
try try
{ {
if (frame_obj != NULL) 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 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 yield an incorrect value when symbol is not local to FRAME_INFO (this
can happen with nested functions). */ 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_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 /* Given a symbol, and a symbol_object that has previously been

View File

@ -957,7 +957,6 @@ typy_template_argument (PyObject *self, PyObject *args)
const struct block *block = NULL; const struct block *block = NULL;
PyObject *block_obj = NULL; PyObject *block_obj = NULL;
struct symbol *sym; struct symbol *sym;
struct value *val = NULL;
if (! PyArg_ParseTuple (args, "i|O", &argno, &block_obj)) if (! PyArg_ParseTuple (args, "i|O", &argno, &block_obj))
return NULL; return NULL;
@ -1014,16 +1013,19 @@ typy_template_argument (PyObject *self, PyObject *args)
return NULL; return NULL;
} }
PyObject *result = nullptr;
try 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return value_to_value_object (val); return result;
} }
static PyObject * static PyObject *
@ -1189,6 +1191,7 @@ typy_optimized_out (PyObject *self, PyObject *args)
{ {
struct type *type = ((type_object *) self)->type; struct type *type = ((type_object *) self)->type;
scoped_value_mark free_values;
return value_to_value_object (value::allocate_optimized_out (type)); return value_to_value_object (value::allocate_optimized_out (type));
} }

View File

@ -365,7 +365,6 @@ static PyObject *
pending_framepy_read_register (PyObject *self, PyObject *args) pending_framepy_read_register (PyObject *self, PyObject *args)
{ {
pending_frame_object *pending_frame = (pending_frame_object *) self; pending_frame_object *pending_frame = (pending_frame_object *) self;
struct value *val = NULL;
int regnum; int regnum;
PyObject *pyo_reg_id; 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)) if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
return nullptr; return nullptr;
PyObject *result = nullptr;
try try
{ {
scoped_value_mark free_values;
/* Fetch the value associated with a register, whether it's /* Fetch the value associated with a register, whether it's
a real register or a so called "user" register, like "pc", a real register or a so called "user" register, like "pc",
which maps to a real register. In the past, which maps to a real register. In the past,
get_frame_register_value() was used here, which did not get_frame_register_value() was used here, which did not
handle the user register case. */ 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) if (val == NULL)
PyErr_Format (PyExc_ValueError, PyErr_Format (PyExc_ValueError,
"Cannot read register %d from frame.", "Cannot read register %d from frame.",
regnum); regnum);
else
result = value_to_value_object (val);
} }
catch (const gdb_exception &except) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return val == NULL ? NULL : value_to_value_object (val); return result;
} }
/* Implementation of /* Implementation of

View File

@ -1559,18 +1559,20 @@ valpy_nonzero (PyObject *self)
static PyObject * static PyObject *
valpy_invert (PyObject *self) valpy_invert (PyObject *self)
{ {
struct value *val = NULL; PyObject *result = nullptr;
try 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return value_to_value_object (val); return result;
} }
/* Implements left shift for value objects. */ /* Implements left shift for value objects. */
@ -1774,32 +1776,10 @@ valpy_float (PyObject *self)
return PyFloat_FromDouble (d); return PyFloat_FromDouble (d);
} }
/* Returns an object for a value which is released from the all_values chain, /* Returns an object for a value, without releasing it from the
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
all_values chain. */ all_values chain. */
PyObject * PyObject *
value_to_value_object_no_release (struct value *val) value_to_value_object (struct value *val)
{ {
value_object *val_obj; value_object *val_obj;
@ -1926,21 +1906,23 @@ PyObject *
gdbpy_history (PyObject *self, PyObject *args) gdbpy_history (PyObject *self, PyObject *args)
{ {
int i; int i;
struct value *res_val = NULL; /* Initialize to appease gcc warning. */
if (!PyArg_ParseTuple (args, "i", &i)) if (!PyArg_ParseTuple (args, "i", &i))
return NULL; return NULL;
PyObject *result = nullptr;
try 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_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 /* 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)) if (!PyArg_ParseTuple (args, "s", &varname))
return NULL; return NULL;
PyObject *result = nullptr;
bool found = false;
try try
{ {
struct internalvar *var = lookup_only_internalvar (varname); struct internalvar *var = lookup_only_internalvar (varname);
if (var != NULL) if (var != NULL)
{ {
scoped_value_mark free_values;
res_val = value_of_internalvar (gdbpy_enter::get_gdbarch (), var); res_val = value_of_internalvar (gdbpy_enter::get_gdbarch (), var);
if (res_val->type ()->code () == TYPE_CODE_VOID) if (res_val->type ()->code () == TYPE_CODE_VOID)
res_val = NULL; res_val = NULL;
else
{
found = true;
result = value_to_value_object (res_val);
}
} }
} }
catch (const gdb_exception &except) catch (const gdb_exception &except)
@ -2004,10 +1994,10 @@ gdbpy_convenience_variable (PyObject *self, PyObject *args)
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
if (res_val == NULL) if (result == nullptr && !found)
Py_RETURN_NONE; Py_RETURN_NONE;
return value_to_value_object (res_val); return result;
} }
/* Set the value of a convenience variable. */ /* Set the value of a convenience variable. */

View File

@ -423,6 +423,7 @@ python_xmethod_worker::do_get_result_type (value *obj,
return EXT_LANG_RC_OK; return EXT_LANG_RC_OK;
} }
scoped_value_mark free_values;
obj_type = check_typedef (obj->type ()); obj_type = check_typedef (obj->type ());
this_type = check_typedef (type_object_to_type (m_this_type)); this_type = check_typedef (type_object_to_type (m_this_type));
if (obj_type->code () == TYPE_CODE_PTR) if (obj_type->code () == TYPE_CODE_PTR)

View File

@ -437,7 +437,6 @@ PyObject *symbol_to_symbol_object (struct symbol *sym);
PyObject *block_to_block_object (const struct block *block, PyObject *block_to_block_object (const struct block *block,
struct objfile *objfile); struct objfile *objfile);
PyObject *value_to_value_object (struct value *v); 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 *type_to_type_object (struct type *);
PyObject *frame_info_to_frame_object (frame_info_ptr frame); PyObject *frame_info_to_frame_object (frame_info_ptr frame);
PyObject *symtab_to_linetable_object (PyObject *symtab); PyObject *symtab_to_linetable_object (PyObject *symtab);

View File

@ -970,22 +970,24 @@ static PyObject *
gdbpy_parse_and_eval (PyObject *self, PyObject *args) gdbpy_parse_and_eval (PyObject *self, PyObject *args)
{ {
const char *expr_str; const char *expr_str;
struct value *result = NULL;
if (!PyArg_ParseTuple (args, "s", &expr_str)) if (!PyArg_ParseTuple (args, "s", &expr_str))
return NULL; return NULL;
PyObject *result = nullptr;
try try
{ {
gdbpy_allow_threads allow_threads; 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) catch (const gdb_exception &except)
{ {
GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION (except);
} }
return value_to_value_object (result); return result;
} }
/* Implementation of gdb.invalidate_cached_frames. */ /* Implementation of gdb.invalidate_cached_frames. */

View File

@ -0,0 +1,35 @@
/* This testcase is part of GDB, the GNU debugger.
Copyright 2023 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/>. */
typedef int pp_int;
int break_function()
{
return 0;
}
struct container
{
pp_int p_i;
int i;
};
int main()
{
struct container c = { 10, 5 };
return break_function();
}

View File

@ -0,0 +1,40 @@
# Copyright (C) 2023 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 casting of a gdb.Value inside a pretty printer.
load_lib gdb-python.exp
require allow_python_tests
standard_testfile
if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
return -1
}
if ![runto break_function] {
return -1
}
set remote_python_file [gdb_remote_download host \
${srcdir}/${subdir}/${testfile}.py]
gdb_test_no_output "source ${remote_python_file}" \
"source ${testfile}.py"
gdb_test "up" "#1.*main.*"
gdb_test "info locals" "c = {p_i = 10p, i = 5}"

View File

@ -0,0 +1,28 @@
# Copyright (C) 2023 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/>.
class PpIntPrinter(object):
def __init__(self, val):
self.val = val
def to_string(self):
val = self.val.cast(self.val.type)
return "%dp" % int(val)
pp = gdb.printing.RegexpCollectionPrettyPrinter("pp-cast")
pp.add_printer("pp_int", "^pp_int$", PpIntPrinter)
gdb.printing.register_pretty_printer(gdb.current_objfile(), pp)