Always create a new value object in valpy_do_cast
Commit Message
In case a pretty printer casts a value to the same type, so value_cast()
returns the same object again, you get this simplified situation:
{
struct value *val = ((value_object *) self)->value;
scoped_value_mark free_values;
res_val = value_cast (type, val); // this returns val
result = value_to_value_object (res_val);
}
So value_to_value_object() removes a value at or before the free_values
marker.
And if this happens inside a pretty printer, then the self value_object
was created with value_to_value_object_no_release(), so the original value
is still in all_values, at the last position.
Putting this together means that the value_to_value_object() releases
the exact value object that is referenced by the free_values marker,
and at its destruction value_free_to_mark() clears all_values completely.
If the variable that is pretty printed is part of a struct, and this
struct is again referenced afterwards for other members, then it will
try to access already freed objects:
$ valgrind ./gdb pp-cast
==16306== Memcheck, a memory error detector
==16306== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==16306== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==16306== Command: ./gdb pp-cast
==16306==
Reading symbols from pp-cast...
(gdb) b break_function
Breakpoint 1 at 0x4004a4: file pp-cast.cpp, line 6.
(gdb) r
Starting program: pp-cast
Breakpoint 1, break_function () at pp-cast.cpp:6
6 return 0;
(gdb) up
18 return break_function();
(gdb) info locals
==16306== Invalid read of size 1
==16306== at 0xA47EA0: value_contents_for_printing(value*) (value.c:1267)
==16306== by 0x70E069: cp_print_value_fields(value*, ui_file*, int, value_print_options const*, type**, int) (cp-valprint.c:192)
==16306== by 0x6BDD01: c_value_print_inner(value*, ui_file*, int, value_print_options const*) (c-valprint.c:396)
==16306== by 0xA3EE4F: common_val_print(value*, ui_file*, int, value_print_options const*, language_defn const*) (valprint.c:1074)
==16306== by 0x8D9B31: print_variable_and_value(char const*, symbol*, frame_info_ptr, ui_file*, int) (printcmd.c:2416)
==16306== by 0x99DCD5: print_variable_and_value_data::operator()(char const*, symbol*) (stack.c:2353)
==16306== by 0x99470E: iterate_over_block_local_vars(block const*, gdb::function_view<void (char const*, symbol*)>) (function-view.h:289)
==16306== by 0x99E071: print_frame_local_vars(frame_info_ptr, bool, char const*, char const*, int, ui_file*) (stack.c:2427)
==16306== by 0x99E2E0: info_locals_command(char const*, int) (stack.c:2508)
==16306== by 0x6CBDF5: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:2543)
==16306== by 0x9F5178: execute_command(char const*, int) (top.c:700)
==16306== by 0x7A3F23: command_handler(char const*) (event-top.c:616)
==16306== Address 0x12701ad4 is 4 bytes inside a block of size 176 free'd
==16306== at 0x4A09430: free (vg_replace_malloc.c:446)
==16306== by 0xA462EC: value_free_to_mark(value const*) (value.h:114)
==16306== by 0x92494D: valpy_do_cast(_object*, _object*, exp_opcode) (value.h:796)
==16306== by 0xDD32C5: method_vectorcall_VARARGS (descrobject.c:330)
==16306== by 0xBD98AE: PyObject_Vectorcall (pycore_call.h:92)
==16306== by 0x5E9887: _PyEval_EvalFrameDefault (ceval.c:4772)
==16306== by 0xCA9354: _PyEval_Vector (pycore_ceval.h:73)
==16306== by 0xBD8FFF: object_vacall (pycore_call.h:92)
==16306== by 0xBDC401: PyObject_CallMethodObjArgs (call.c:879)
==16306== by 0x9115D2: pretty_print_one_value(_object*, value**) (py-prettyprint.c:205)
==16306== by 0x91198B: gdbpy_apply_pretty_printer(_object*, ui_file*, int, value_print_options const*, language_defn const*, gdbarch*) (py-prettyprint.c:290)
==16306== by 0x912598: gdbpy_apply_val_pretty_printer(extension_language_defn const*, value*, ui_file*, int, value_print_options const*, language_defn const*) (py-prettyprint.c:627)
Fixed by creating an explicit copy of the value if the cast function
returned the original value again.
---
gdb/python/py-value.c | 3 ++
gdb/testsuite/gdb.python/py-pp-cast.c | 35 +++++++++++++++++++++
gdb/testsuite/gdb.python/py-pp-cast.exp | 41 +++++++++++++++++++++++++
gdb/testsuite/gdb.python/py-pp-cast.py | 28 +++++++++++++++++
4 files changed, 107 insertions(+)
create mode 100644 gdb/testsuite/gdb.python/py-pp-cast.c
create mode 100644 gdb/testsuite/gdb.python/py-pp-cast.exp
create mode 100644 gdb/testsuite/gdb.python/py-pp-cast.py
Comments
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
Hannes> In case a pretty printer casts a value to the same type, so value_cast()
Hannes> returns the same object again, you get this simplified situation:
Hannes> {
Hannes> struct value *val = ((value_object *) self)->value;
Hannes> scoped_value_mark free_values;
Hannes> res_val = value_cast (type, val); // this returns val
Hannes> result = value_to_value_object (res_val);
Hannes> }
Hannes> So value_to_value_object() removes a value at or before the free_values
Hannes> marker.
Something seems off about this to me.
value_to_value_object does this:
val_obj->value = release_value (val).release ();
release_value is like an "incref" but its actual semantics are: if the
value is on all_values, remove it and return it; otherwise incref. That
way the caller always (1) is assured that the value isn't on all_values,
and (2) always gets a new reference.
So, I would expect that call to have released it from all_values and
therefore it would not be destroyed.
Furthermore, no matter what, I'd expect a gdb.Value to hold an owning
reference to the underlying value, so it still shouldn't be destroyed by
value_free_to_mark.
I'm not doubting there's a bug here, but I don't understand how it comes
about. Also, I would rather not fix it the way it is done in this
patch, because I think it is preferable for users of values not to have
to know about whether or not a given API might return the same value.
Tom
Am Sonntag, 22. Januar 2023, 18:49:55 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> In case a pretty printer casts a value to the same type, so value_cast()
> Hannes> returns the same object again, you get this simplified situation:
>
> Hannes> {
> Hannes> struct value *val = ((value_object *) self)->value;
> Hannes> scoped_value_mark free_values;
> Hannes> res_val = value_cast (type, val); // this returns val
> Hannes> result = value_to_value_object (res_val);
> Hannes> }
>
> Hannes> So value_to_value_object() removes a value at or before the free_values
> Hannes> marker.
>
> Something seems off about this to me.
>
> value_to_value_object does this:
>
> val_obj->value = release_value (val).release ();
>
> release_value is like an "incref" but its actual semantics are: if the
> value is on all_values, remove it and return it; otherwise incref. That
> way the caller always (1) is assured that the value isn't on all_values,
> and (2) always gets a new reference.
>
> So, I would expect that call to have released it from all_values and
> therefore it would not be destroyed.
The problem isn't the value that's reference by gdb.Value, instead
one of the other values in all_values before it.
But release_value removes it from all_values, and it was the exact value
that the scoped_value_mark free_values instance was using as the mark point,
and since it was then missing, all_values was cleared completely.
And one of those earlier values is still used by the printing later on.
> Furthermore, no matter what, I'd expect a gdb.Value to hold an owning
> reference to the underlying value, so it still shouldn't be destroyed by
> value_free_to_mark.
>
> I'm not doubting there's a bug here, but I don't understand how it comes
> about. Also, I would rather not fix it the way it is done in this
> patch, because I think it is preferable for users of values not to have
> to know about whether or not a given API might return the same value.
It's also weird for me that users of these APIs need to know this, that's
why the way scoped_value_mark works was very surprising for me.
Hannes
>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
Hannes> The problem isn't the value that's reference by gdb.Value, instead
Hannes> one of the other values in all_values before it.
Hannes> But release_value removes it from all_values, and it was the exact value
Hannes> that the scoped_value_mark free_values instance was using as the mark point,
Hannes> and since it was then missing, all_values was cleared completely.
Hannes> And one of those earlier values is still used by the printing later on.
OMG. I think this is a latent bug in the value-mark API that's been
there since... well I don't know exactly when, maybe forever. So, nice
find!
I think what we probably need is to change how the value_mark API works,
so that it isn't relying on a value pointer being in all_values, but
rather return some other type/object that isn't invalidated in the same
way.
Tom
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
Hannes> In case a pretty printer casts a value to the same type, so value_cast()
Hannes> returns the same object again, you get this simplified situation:
[...]
Hannes> $ valgrind ./gdb pp-cast
I tried to reproduce this -- I was hoping to reuse your test case for my
patch -- but I couldn't. That seems very strange to me, I wonder what
could be going wrong.
Tom
On 1/22/23 15:50, Tom Tromey wrote:
>>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> The problem isn't the value that's reference by gdb.Value, instead
> Hannes> one of the other values in all_values before it.
>
> Hannes> But release_value removes it from all_values, and it was the exact value
> Hannes> that the scoped_value_mark free_values instance was using as the mark point,
> Hannes> and since it was then missing, all_values was cleared completely.
> Hannes> And one of those earlier values is still used by the printing later on.
>
> OMG. I think this is a latent bug in the value-mark API that's been
> there since... well I don't know exactly when, maybe forever. So, nice
> find!
>
> I think what we probably need is to change how the value_mark API works,
> so that it isn't relying on a value pointer being in all_values, but
> rather return some other type/object that isn't invalidated in the same
> way.
Is there an advantage of that value chain thing vs using value_ref_ptr
throughout? Not replace all `value *` with value_ref_ptr, but have
functions return and accept value_ref_ptr where that makes sense.
Simon
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>> I think what we probably need is to change how the value_mark API works,
>> so that it isn't relying on a value pointer being in all_values, but
>> rather return some other type/object that isn't invalidated in the same
>> way.
Simon> Is there an advantage of that value chain thing vs using value_ref_ptr
Simon> throughout? Not replace all `value *` with value_ref_ptr, but have
Simon> functions return and accept value_ref_ptr where that makes sense.
The chain is used to implement the watchpoint semantics. It's needed, I
think, but it would be better if it were opt-in -- that is, some caller
has to request it explicitly. However that requires switching to
value_ref_ptr everywhere...
.. but please don't do that, I'm nearly done with a series to move value
to value.h and use methods, and it's reasonably large and would be
painful to rebase.
Meanwhile I think we have a fix for this bug, the problem is finding a
test case. Though... the one we have does fail for Hannes so perhaps
that's enough.
Tom
Am Donnerstag, 9. Februar 2023 um 00:52:49 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> In case a pretty printer casts a value to the same type, so value_cast()
> Hannes> returns the same object again, you get this simplified situation:
> [...]
>
> Hannes> $ valgrind ./gdb pp-cast
>
> I tried to reproduce this -- I was hoping to reuse your test case for my
> patch -- but I couldn't. That seems very strange to me, I wonder what
> could be going wrong.
If you did source the py-pp-cast.py pretty printer file and can't reproduce
this, then I don't know what could be different.
I've tested it quite a few times, on both Linux and Windows, and it always
happened for me.
Regards
Hannes
@@ -815,6 +815,9 @@ valpy_do_cast (PyObject *self, PyObject *args, enum exp_opcode op)
res_val = value_cast (type, val);
}
+ if (res_val == val)
+ res_val = value_copy (val);
+
result = value_to_value_object (res_val);
}
catch (const gdb_exception &except)
new file mode 100644
@@ -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();
+}
new file mode 100644
@@ -0,0 +1,41 @@
+# 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
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+ return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+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}"
new file mode 100644
@@ -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)