[PATCH] Always create a new value object in valpy_do_cast

[PATCH] Always create a new value object in valpy_do_cast

[Hannes Domani via 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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Tom Tromey]

>>>>> "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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Hannes Domani via Gdb-patches]

 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

Re: [PATCH] Always create a new value object in valpy_do_cast

[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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Simon Marchi 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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Tom Tromey]

>>>>> "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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Tom Tromey]

>>>>> "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

Re: [PATCH] Always create a new value object in valpy_do_cast

[Hannes Domani via Gdb-patches]

 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