* [PATCH] Always create a new value object in valpy_do_cast [not found] <20230118172740.2171-1-ssbssa.ref@yahoo.de> @ 2023-01-18 17:27 ` Hannes Domani 2023-01-22 17:49 ` Tom Tromey 2023-02-08 23:52 ` Tom Tromey 0 siblings, 2 replies; 8+ messages in thread From: Hannes Domani @ 2023-01-18 17:27 UTC (permalink / raw) To: gdb-patches 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 diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index dcc92e51b60..51e00a99481 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -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) diff --git a/gdb/testsuite/gdb.python/py-pp-cast.c b/gdb/testsuite/gdb.python/py-pp-cast.c new file mode 100644 index 00000000000..72716a79dd4 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-pp-cast.c @@ -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(); +} diff --git a/gdb/testsuite/gdb.python/py-pp-cast.exp b/gdb/testsuite/gdb.python/py-pp-cast.exp new file mode 100644 index 00000000000..0842babaacc --- /dev/null +++ b/gdb/testsuite/gdb.python/py-pp-cast.exp @@ -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}" diff --git a/gdb/testsuite/gdb.python/py-pp-cast.py b/gdb/testsuite/gdb.python/py-pp-cast.py new file mode 100644 index 00000000000..b171a919c70 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-pp-cast.py @@ -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) -- 2.35.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-01-18 17:27 ` [PATCH] Always create a new value object in valpy_do_cast Hannes Domani @ 2023-01-22 17:49 ` Tom Tromey 2023-01-22 18:22 ` Hannes Domani 2023-02-08 23:52 ` Tom Tromey 1 sibling, 1 reply; 8+ messages in thread From: Tom Tromey @ 2023-01-22 17:49 UTC (permalink / raw) To: Hannes Domani via Gdb-patches; +Cc: Hannes Domani >>>>> "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-01-22 17:49 ` Tom Tromey @ 2023-01-22 18:22 ` Hannes Domani 2023-01-22 20:50 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Hannes Domani @ 2023-01-22 18:22 UTC (permalink / raw) To: Hannes Domani via Gdb-patches, Tom Tromey 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-01-22 18:22 ` Hannes Domani @ 2023-01-22 20:50 ` Tom Tromey 2023-02-09 2:08 ` Simon Marchi 0 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2023-01-22 20:50 UTC (permalink / raw) To: Hannes Domani; +Cc: Hannes Domani via Gdb-patches, Tom Tromey >>>>> "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-01-22 20:50 ` Tom Tromey @ 2023-02-09 2:08 ` Simon Marchi 2023-02-09 3:35 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Simon Marchi @ 2023-02-09 2:08 UTC (permalink / raw) To: Tom Tromey, Hannes Domani; +Cc: Hannes Domani via Gdb-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-02-09 2:08 ` Simon Marchi @ 2023-02-09 3:35 ` Tom Tromey 0 siblings, 0 replies; 8+ messages in thread From: Tom Tromey @ 2023-02-09 3:35 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, Hannes Domani, Hannes Domani via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-01-18 17:27 ` [PATCH] Always create a new value object in valpy_do_cast Hannes Domani 2023-01-22 17:49 ` Tom Tromey @ 2023-02-08 23:52 ` Tom Tromey 2023-02-09 6:06 ` Hannes Domani 1 sibling, 1 reply; 8+ messages in thread From: Tom Tromey @ 2023-02-08 23:52 UTC (permalink / raw) To: Hannes Domani via Gdb-patches; +Cc: Hannes Domani >>>>> "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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Always create a new value object in valpy_do_cast 2023-02-08 23:52 ` Tom Tromey @ 2023-02-09 6:06 ` Hannes Domani 0 siblings, 0 replies; 8+ messages in thread From: Hannes Domani @ 2023-02-09 6:06 UTC (permalink / raw) To: Hannes Domani via Gdb-patches, Tom Tromey 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-09 6:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230118172740.2171-1-ssbssa.ref@yahoo.de> 2023-01-18 17:27 ` [PATCH] Always create a new value object in valpy_do_cast Hannes Domani 2023-01-22 17:49 ` Tom Tromey 2023-01-22 18:22 ` Hannes Domani 2023-01-22 20:50 ` Tom Tromey 2023-02-09 2:08 ` Simon Marchi 2023-02-09 3:35 ` Tom Tromey 2023-02-08 23:52 ` Tom Tromey 2023-02-09 6:06 ` Hannes Domani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).