get_one_instruction: clear "cont" cache on mem/reg changed (#1828)

* get_one_instruction: clear "cont" cache on mem/reg changed

Fixes #1818.

Note that this makes a substantial change: it changes all caches that
are refreshed on `gdb.ContinueEvent` to also be cleared on memory/regs
changed.

This change is needed so that the `get_one_instruction` function which
uses this cache will get its cache cleared when user invokes a command
that changes memory or registers.

While this may sound as too big change: we are changing the whole "cont"
cache to be cleared on two additional events, this should not be an
issue. This is because:
1. We should notice it if we start clearing an important cache too often
2. The "cont" cache is currently only used by the `get_one_instruction`
   at this moment.

The 2) also creates a question: when should one use "cont" vs "start"
caches? It is not so clear to me right now.

* Add test for issue #1818

* Clear caches on MemoryChanged events from gdblib.write

Regarding the last part:

Interestingly implementing tests here uncovered another bug: the gdblib.memory.write(..) or rather the gdb.selected_inferior().write_memory(...) API used there does not trigger a gdb.MemoryChanged event. As a result, we never cleared certain caches that should have been cleared when the user used that API.

I have added two tests here, one changes the instructions at $RIP to nops via gdblib.memory.write(..) and another via executing the patch $rip nop;nop;nop;nop;nop command. As a result, we test both scenarios: 1) when we depend on memory changed event being fired via GDB to clear caches; and 2) when we depend on gdblib.memory.write(..) to clear the caches.

This PR also makes a fix to the gdblib.memory.write(..) to actually clear caches that depend on (or rather: are hooked to in order to be cleared) memory changed events.

pwndbg/gdblib/hooks.py

@@ -62,7 +62,11 @@ pwndbg.lib.cache.connect_clear_caching_events(
         "exit": (pwndbg.gdblib.events.exit,),
         "objfile": (pwndbg.gdblib.events.new_objfile,),
         "start": (pwndbg.gdblib.events.start,),
-        "cont": (pwndbg.gdblib.events.cont,),
+        "cont": (
+            pwndbg.gdblib.events.cont,
+            pwndbg.gdblib.events.mem_changed,
+            pwndbg.gdblib.events.reg_changed,
+        ),
         "thread": (pwndbg.gdblib.events.thread,),
         "prompt": (pwndbg.gdblib.events.before_prompt,),
         "forever": (),

pwndbg/gdblib/memory.py

@@ -96,6 +96,12 @@ def write(addr, data) -> None:
     # Throws an exception if can't access memory
     gdb.selected_inferior().write_memory(addr, data)
 
+    # Clear caches which are hooked to gdb.MemoryChanged events
+    # We do this explicitly because `write_memory` does not fire off
+    # the gdb.MemoryChanged event (see #1818)
+    pwndbg.lib.cache.clear_cache("stop")
+    pwndbg.lib.cache.clear_cache("cont")
+
 
 def peek(address):
     """peek(address) -> str

pwndbg/lib/cache.py

@@ -62,7 +62,7 @@ class _CacheUntilEvent:
 
     def connect_event_hooks(self, event_hooks) -> None:
         """
-        A given cache until event may require multiple debugger events
+        A given _CacheUntilEvent object may require multiple debugger events
         to be handled properly. E.g. our `stop` cache needs to be handled
         by `stop`, `mem_changed` and `reg_changed` events.
         """

tests/gdb-tests/tests/test_context_commands.py

@@ -254,3 +254,40 @@ def test_context_disasm_works_properly_with_disasm_flavor_switch(start_binary):
         out[1] == "──────────────────────[ DISASM / x86-64 / set emulate on ]──────────────────────"
     )
     assert_att(out)
+
+
+@pytest.mark.parametrize("patch_or_api", (True, False))
+def test_context_disasm_proper_render_on_mem_change_issue_1818(start_binary, patch_or_api):
+    start_binary(SYSCALLS_BINARY)
+
+    old = gdb.execute("context disasm", to_string=True).split("\n")
+
+    # Just a sanity check
+    assert old[0] == "LEGEND: STACK | HEAP | CODE | DATA | RWX | RODATA"
+    assert "mov    eax, 0" in old[2]
+    assert "mov    edi, 0x1337" in old[3]
+    assert "mov    esi, 0xdeadbeef" in old[4]
+    assert "mov    ecx, 0x10" in old[5]
+    assert "syscall" in old[6]
+
+    # 5 bytes because 'mov eax, 0' is 5 bytes long
+    if patch_or_api:
+        gdb.execute("patch $rip nop;nop;nop;nop;nop", to_string=True)
+    else:
+        # Do the same, but through write API
+        pwndbg.gdblib.memory.write(pwndbg.gdblib.regs.rip, b"\x90" * 5)
+
+    # Actual test: we expect the read memory to be different now ;)
+    # (and not e.g. returned incorrectly from a not cleared cache)
+    new = gdb.execute("context disasm", to_string=True).split("\n")
+
+    assert new[0] == "LEGEND: STACK | HEAP | CODE | DATA | RWX | RODATA"
+    assert "nop" in new[2]
+    assert "nop" in new[3]
+    assert "nop" in new[4]
+    assert "nop" in new[5]
+    assert "nop" in new[6]
+    assert "mov    edi, 0x1337" in new[7]
+    assert "mov    esi, 0xdeadbeef" in new[8]
+    assert "mov    ecx, 0x10" in new[9]
+    assert "syscall" in new[10]