Fix memory.poke and make memory.peek return bytearray (#2483)

Fixes #2472 where poke could write incorrect bytes to memory when testing if certain address is writable or not (see longer description below).

Summary of changes:

*    Changes `{aglib,gdblib}.memory.peek` to return a `bytearray` instead of `str` (which made no sense)
*    Add tests for multiple use cases of `gdblib.memory.{peek,poke}`

Longer description:

Before this commit, the `memory.poke` implementation could end up writing a different byte to memory then the byte that was initially there.

This is because it used `memory.peek` which returned a `str` and this str was then encoded to utf-8 to get bytes back again for `memory.write` used by `memory.poke`.

This resulted in writing different bytes to memory then the byte that was initially read from the given address. Below is an example.

If `memory.peek` read the `b'\xa9'` byte, then this was converted to a `'©'` string via chr(b'\xa9'[0]) which was returned from peek. Then, this was converted back to bytes in poke with utf-8 encoding which resulted in `b'\xc2\xa9'` bytes.

```
In [4]: bytes(chr(b'\xa9'[0]), 'utf-8')
Out[4]: b'\xc2\xa9'
```

During all this I also tried to change the `peek/poke` to only pass on a `gdb.MemoryError`. This however uncovered some other bugs that have to be fixed in another PR/commit.

pwndbg/aglib/memory.py

@@ -65,8 +65,8 @@ def write(addr: int, data: str | bytes | bytearray) -> None:
     pwndbg.dbg.selected_inferior().write_memory(address=addr, data=bytearray(data), partial=False)
 
 
-def peek(address: int) -> str | None:
-    """peek(address) -> str
+def peek(address: int) -> bytearray | None:
+    """peek(address) -> bytearray
 
     Read one byte from the specified address.
 
@@ -74,11 +74,11 @@ def peek(address: int) -> str | None:
         address(int): Address to read
 
     Returns:
-        :class:`str`: A single byte of data, or ``None`` if the
+        :class:`bytearray`: A single byte of data, or ``None`` if the
         address cannot be read.
     """
     try:
-        return chr(read(address, 1)[0])
+        return read(address, 1)
     except Exception:
         pass
     return None

pwndbg/gdblib/memory.py

@@ -111,8 +111,8 @@ def write(addr: int, data: str | bytes | bytearray) -> None:
     gdb.selected_inferior().write_memory(addr, data)
 
 
-def peek(address: int) -> str | None:
-    """peek(address) -> str
+def peek(address: int) -> bytearray | None:
+    """peek(address) -> bytearray
 
     Read one byte from the specified address.
 
@@ -120,11 +120,11 @@ def peek(address: int) -> str | None:
         address(int): Address to read
 
     Returns:
-        :class:`str`: A single byte of data, or ``None`` if the
+        :class:`bytearray`: A single byte of data, or ``None`` if the
         address cannot be read.
     """
     try:
-        return chr(read(address, 1)[0])
+        return read(address, 1)
     except Exception:
         pass
     return None

tests/gdb-tests/tests/test_memory.py

@@ -35,6 +35,55 @@ def test_memory_read_write(start_binary):
     )
 
 
+def test_memory_peek_poke(start_binary):
+    """
+    This tests ensures that doing a peek, poke, peek round-robin operations
+    ends up with the same value in memory.
+    It also sets a given byte in memory via memory.write
+    and tests all single-byte values.
+    """
+    start_binary(REFERENCE_BINARY)
+
+    # Address 0 is not mapped, so peek should return None
+    # and poke should fail
+    assert pwndbg.gdblib.memory.poke(0) is False
+    assert pwndbg.gdblib.memory.peek(0) is None
+
+    stack_addr = pwndbg.gdblib.regs.rsp
+
+    for v in range(256):
+        data = bytearray([v, 0, 0, 0])
+        pwndbg.gdblib.memory.write(stack_addr, data)
+
+        # peek should return the first byte
+        assert pwndbg.gdblib.memory.peek(stack_addr) == bytearray([v])
+
+        # Now poke it!
+        assert pwndbg.gdblib.memory.poke(stack_addr) is True
+
+        # Now make sure poke did not change the underlying bytes
+        assert pwndbg.gdblib.memory.read(stack_addr, 4) == data
+
+    # TODO/FIXME: Fix peek/poke exception handling and uncomment this!
+    """
+    # Ensure that peek and poke correctly raises exceptions
+    # when incorrect argument type is passed
+    for not_parsable_as_int in (b"asdf", "asdf"):
+        with pytest.raises(ValueError):
+            pwndbg.gdblib.memory.peek(not_parsable_as_int)
+
+    for not_parsable_as_int in (b"asdf", "asdf"):
+        with pytest.raises(ValueError):
+            pwndbg.gdblib.memory.poke(not_parsable_as_int)
+    """
+
+    # Acceptable inputs (not great; maybe we should ban them?)
+    # Note: they return 0 because the address 0 is not mapped
+    assert pwndbg.gdblib.memory.peek(0.0) is None
+    assert pwndbg.gdblib.memory.peek("0") is None
+    assert pwndbg.gdblib.memory.peek(b"0") is None
+
+
 def test_fetch_struct_as_dictionary(start_binary):
     """
     Test pwndbg.gdblib.memory.fetch_struct_as_dictionary()