From 9687a5af1d712da41992cba8d241ddef8bdc50ec Mon Sep 17 00:00:00 2001
From: iximeow <me@iximeow.net>
Date: Sat, 21 Aug 2021 13:13:26 -0700
Subject: clarify inaccurate 32/16-bit `call/jmp [mem]` mem_size

---
 src/protected_mode/mod.rs      | 14 +++++++++++---
 src/real_mode/mod.rs           | 18 +++++++++++-------
 test/long_mode/operand.rs      |  3 +++
 test/protected_mode/operand.rs |  4 ++++
 test/real_mode/operand.rs      |  7 +++++--
 5 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/protected_mode/mod.rs b/src/protected_mode/mod.rs
index 8381d68..79eb1b1 100644
--- a/src/protected_mode/mod.rs
+++ b/src/protected_mode/mod.rs
@@ -4223,6 +4223,15 @@ impl Instruction {
     /// the corresponding `MemoryAccessSize` may report that the size of accessed memory is
     /// indeterminate; this is the case for `xsave/xrestor`-style instructions whose operation size
     /// varies based on physical processor.
+    ///
+    /// ## NOTE
+    ///
+    /// the reported size is correct for displayed operand sizes (`word [ptr]` will have a
+    /// `MemoryAccessSize` indicating two bytes) but is _not_ sufficient to describe all accesses
+    /// of all instructions. the most notable exception is for operand-size-prefixed `call`, where
+    /// `66ff10` is the instruction `call word [eax]`, but will push a four-byte `eip`. this same
+    /// imprecision exists for `jmp word [mem]` as well. tools must account for these inconsistent
+    /// sizes internally.
     pub fn mem_size(&self) -> Option<MemoryAccessSize> {
         if self.mem_size != 0 {
             Some(MemoryAccessSize { size: self.mem_size })
@@ -7516,9 +7525,8 @@ fn read_operands<T: Reader<<Arch as yaxpeax_arch::Arch>::Address, <Arch as yaxpe
                     return Err(DecodeError::InvalidOperand);
                 }
             } else {
-                if opcode == Opcode::CALL || opcode == Opcode::JMP {
-                    instruction.mem_size = 4;
-                } else if opcode == Opcode::PUSH || opcode == Opcode::POP {
+                if opcode == Opcode::CALL || opcode == Opcode::JMP ||
+                    opcode == Opcode::PUSH || opcode == Opcode::POP {
                     if instruction.prefixes.operand_size() {
                         instruction.mem_size = 2;
                     } else {
diff --git a/src/real_mode/mod.rs b/src/real_mode/mod.rs
index 548c42e..8a7e453 100644
--- a/src/real_mode/mod.rs
+++ b/src/real_mode/mod.rs
@@ -4223,6 +4223,15 @@ impl Instruction {
     /// the corresponding `MemoryAccessSize` may report that the size of accessed memory is
     /// indeterminate; this is the case for `xsave/xrestor`-style instructions whose operation size
     /// varies based on physical processor.
+    ///
+    /// ## NOTE
+    ///
+    /// the reported size is correct for displayed operand sizes (`word [ptr]` will have a
+    /// `MemoryAccessSize` indicating two bytes) but is _not_ sufficient to describe all accesses
+    /// of all instructions. the most notable exception is for operand-size-prefixed `call`, where
+    /// `66ff10` is the instruction `call dword [eax]`, but will push a four-byte `eip`. this same
+    /// imprecision exists for `jmp dword [mem]` as well. tools must account for these inconsistent
+    /// sizes internally.
     pub fn mem_size(&self) -> Option<MemoryAccessSize> {
         if self.mem_size != 0 {
             Some(MemoryAccessSize { size: self.mem_size })
@@ -7516,13 +7525,8 @@ fn read_operands<T: Reader<<Arch as yaxpeax_arch::Arch>::Address, <Arch as yaxpe
                     return Err(DecodeError::InvalidOperand);
                 }
             } else {
-                if opcode == Opcode::CALL || opcode == Opcode::JMP {
-                    if instruction.prefixes.operand_size() {
-                        instruction.mem_size = 4;
-                    } else {
-                        instruction.mem_size = 2;
-                    }
-                } else if opcode == Opcode::PUSH || opcode == Opcode::POP {
+                if opcode == Opcode::CALL || opcode == Opcode::JMP ||
+                    opcode == Opcode::PUSH || opcode == Opcode::POP {
                     if instruction.prefixes.operand_size() {
                         instruction.mem_size = 4;
                     } else {
diff --git a/test/long_mode/operand.rs b/test/long_mode/operand.rs
index 4cdaf35..a47e6c8 100644
--- a/test/long_mode/operand.rs
+++ b/test/long_mode/operand.rs
@@ -45,4 +45,7 @@ fn test_implied_memory_width() {
     assert_eq!(mem_size_of(&[0x66, 0x58]), Some(8));
     assert_eq!(mem_size_of(&[0xff, 0xf0]), Some(8));
     assert_eq!(mem_size_of(&[0x66, 0xff, 0xf0]), Some(2));
+    // operand-size prefixed call and jump still reads 8 bytes (prefix ignored)
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x10]), Some(8));
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x20]), Some(8));
 }
diff --git a/test/protected_mode/operand.rs b/test/protected_mode/operand.rs
index a114e69..6eb9ba5 100644
--- a/test/protected_mode/operand.rs
+++ b/test/protected_mode/operand.rs
@@ -43,4 +43,8 @@ fn test_implied_memory_width() {
     assert_eq!(mem_size_of(&[0x66, 0x58]), Some(4));
     assert_eq!(mem_size_of(&[0xff, 0xf0]), Some(4));
     assert_eq!(mem_size_of(&[0x66, 0xff, 0xf0]), Some(2));
+    // unlike 64-bit mode, operand-size prefixed call and jump do have a different size: they read
+    // two bytes.
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x10]), Some(2));
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x20]), Some(2));
 }
diff --git a/test/real_mode/operand.rs b/test/real_mode/operand.rs
index e037fee..7f2b72e 100644
--- a/test/real_mode/operand.rs
+++ b/test/real_mode/operand.rs
@@ -1,5 +1,4 @@
-use yaxpeax_x86::real_mode::{InstDecoder, Operand, RegSpec};
-use yaxpeax_x86::MemoryAccessSize;
+use yaxpeax_x86::real_mode::{InstDecoder};
 
 #[test]
 fn test_implied_memory_width() {
@@ -17,4 +16,8 @@ fn test_implied_memory_width() {
     assert_eq!(mem_size_of(&[0x66, 0x58]), Some(2));
     assert_eq!(mem_size_of(&[0xff, 0xf0]), Some(2));
     assert_eq!(mem_size_of(&[0x66, 0xff, 0xf0]), Some(4));
+    // unlike 64-bit mode, operand-size prefixed call and jump do have a different size: they read
+    // four bytes.
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x10]), Some(4));
+    assert_eq!(mem_size_of(&[0x66, 0xff, 0x20]), Some(4));
 }
-- 
cgit v1.1