[v2,6/6] Use correct inferior in Inferior.read_memory et al

  A user noticed that Inferior.read_memory and a few other Python APIs
will always use the currently selected inferior, not the one passed to
the call.

This patch fixes the bug by arranging to switch to the inferior.  I
found this same issue in several APIs, so this fixes them all.

I also added a few missing calls to INFPY_REQUIRE_VALID to these
methods.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615
---
 gdb/python/py-inferior.c                 | 28 ++++++++++++++++++++++------
 gdb/testsuite/gdb.python/py-inferior.exp | 26 ++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)
  

On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
> A user noticed that Inferior.read_memory and a few other Python APIs
> will always use the currently selected inferior, not the one passed to
> the call.
> 
> This patch fixes the bug by arranging to switch to the inferior.  I
> found this same issue in several APIs, so this fixes them all.
> 
> I also added a few missing calls to INFPY_REQUIRE_VALID to these
> methods.
> 

Approved-By: Pedro Alves <pedro@palves.net>

Thanks,
Pedro Alves

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30615
> ---
>  gdb/python/py-inferior.c                 | 28 ++++++++++++++++++++++------
>  gdb/testsuite/gdb.python/py-inferior.exp | 26 ++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index af8bd8855a3..46b9a32c87d 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -30,6 +30,7 @@
>  #include "gdbsupport/gdb_signals.h"
>  #include "py-event.h"
>  #include "py-stopevent.h"
> +#include "progspace-and-thread.h"
>  #include <unordered_map>
>  
>  using thread_map_t
> @@ -528,11 +529,14 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
>  static PyObject *
>  infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    CORE_ADDR addr, length;
>    gdb::unique_xmalloc_ptr<gdb_byte> buffer;
>    PyObject *addr_obj, *length_obj;
>    static const char *keywords[] = { "address", "length", NULL };
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords,
>  					&addr_obj, &length_obj))
>      return NULL;
> @@ -543,6 +547,9 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        buffer.reset ((gdb_byte *) xmalloc (length));
>  
>        read_memory (addr, buffer.get (), length);
> @@ -565,6 +572,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
>  static PyObject *
>  infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    struct gdb_exception except;
>    Py_ssize_t buf_len;
>    const gdb_byte *buffer;
> @@ -573,6 +581,8 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>    static const char *keywords[] = { "address", "buffer", "length", NULL };
>    Py_buffer pybuf;
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords,
>  					&addr_obj, &pybuf, &length_obj))
>      return NULL;
> @@ -591,6 +601,9 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        write_memory_with_notification (addr, buffer, length);
>      }
>    catch (gdb_exception &ex)
> @@ -604,7 +617,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  }
>  
>  /* Implementation of
> -   gdb.search_memory (address, length, pattern).  ADDRESS is the
> +   Inferior.search_memory (address, length, pattern).  ADDRESS is the
>     address to start the search.  LENGTH specifies the scope of the
>     search from ADDRESS.  PATTERN is the pattern to search for (and
>     must be a Python object supporting the buffer protocol).
> @@ -614,6 +627,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
>  static PyObject *
>  infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>  {
> +  inferior_object *inf = (inferior_object *) self;
>    struct gdb_exception except;
>    CORE_ADDR start_addr, length;
>    static const char *keywords[] = { "address", "length", "pattern", NULL };
> @@ -624,6 +638,8 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>    int found = 0;
>    Py_buffer pybuf;
>  
> +  INFPY_REQUIRE_VALID (inf);
> +
>    if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords,
>  					&start_addr_obj, &length_obj,
>  					&pybuf))
> @@ -656,6 +672,9 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
>  
>    try
>      {
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
> +
>        found = target_search_memory (start_addr, length,
>  				    buffer, pattern_size,
>  				    &found_addr);
> @@ -911,11 +930,8 @@ infpy_get_main_name (PyObject *self, void *closure)
>      {
>        /* This is unfortunate but the implementation of main_name can
>  	 reach into memory.  */
> -      scoped_restore_current_inferior restore_inferior;
> -      set_current_inferior (inf->inferior);
> -
> -      scoped_restore_current_program_space restore_current_progspace;
> -      set_current_program_space (inf->inferior->pspace);
> +      scoped_restore_current_inferior_for_memory restore_inferior
> +	(inf->inferior, any_thread_of_inferior (inf->inferior)->ptid);
>  
>        name = main_name ();
>      }
> diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
> index 8762b6992ca..eb501838117 100644
> --- a/gdb/testsuite/gdb.python/py-inferior.exp
> +++ b/gdb/testsuite/gdb.python/py-inferior.exp
> @@ -91,6 +91,7 @@ gdb_py_test_silent_cmd "python addr = gdb.selected_frame ().read_var ('str')" \
>  gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
>      "<memory at $hex>" \
>      "read str contents"
> +gdb_test "python print(astr\[0\])" .*
>  gdb_py_test_silent_cmd "python a = bytes('a', 'ascii')" "" 0
>  gdb_py_test_silent_cmd "python astr\[1\] = a" "change str" 0
>  gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
> @@ -98,6 +99,10 @@ gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
>  gdb_test "print str" " = \"hallo, testsuite\"" \
>    "ensure str was changed in the inferior"
>  
> +# Add a new inferior here, so we can test that operations work on the
> +# correct inferior.
> +set num [add_inferior]
> +
>  # Test memory search.
>  
>  set hex_number {0x[0-9a-fA-F][0-9a-fA-F]*}
> @@ -115,6 +120,9 @@ with_test_prefix "string" {
>      gdb_test_no_output "py start_addr = search_buf.address"
>      gdb_test_no_output "py length = search_buf.type.sizeof"
>  
> +    # Switch to the new inferior before testing.
> +    gdb_test "inferior $num" ".*" "switch to inferior $num"
> +
>      gdb_test "py print (gdb.inferiors()\[0\].search_memory (start_addr, length, 'aaa'))" \
>  	"${one_pattern_found}" "find string pattern"
>  
> @@ -128,6 +136,24 @@ with_test_prefix "string" {
>  	"${one_pattern_found}" "pattern found at end of range"
>  }
>  
> +# While still in the new inferior, test reading and writing memory
> +# again.
> +gdb_test "python astr = gdb.inferiors()\[0\].read_memory (addr, 5); print(astr)" \
> +    "<memory at $hex>" \
> +    "read str while other inferior selected"
> +gdb_test "python print(astr\[1\])" "b'a'" \
> +    "print a character from the string"
> +gdb_py_test_silent_cmd "python astr\[1\] = b'X'" "change str again" 0
> +gdb_py_test_silent_cmd "python gdb.inferiors()\[0\].write_memory (addr, astr)" \
> +    "write str while other inferior selected" 1
> +
> +gdb_test "inferior 1" ".*" "switch back to inferior 1"
> +
> +gdb_test "print str" " = \"hXllo, testsuite\"" \
> +    "ensure str was changed while other inferior selected"
> +
> +gdb_test_no_output "remove-inferiors $num" "remove-inferiors $num"
> +
>  # Import struct to pack the following patterns.
>  gdb_test_no_output "py from struct import *"
>  
>
  

On 2023-07-14 17:48, Pedro Alves wrote:
> On 2023-07-11 17:14, Tom Tromey via Gdb-patches wrote:
>> A user noticed that Inferior.read_memory and a few other Python APIs
>> will always use the currently selected inferior, not the one passed to
>> the call.
>>
>> This patch fixes the bug by arranging to switch to the inferior.  I
>> found this same issue in several APIs, so this fixes them all.
>>
>> I also added a few missing calls to INFPY_REQUIRE_VALID to these
>> methods.
>>
> 
> Approved-By: Pedro Alves <pedro@palves.net>
> 

I wrote a bunch of text that I ended up deleting ( don't ask :-) ) and then forgot
that I also had some comments on the testcase...  :-P  I'll reply to v3, which I only
noticed now was out already.