From 39620498476a50319346a32a09dc4499132bae29 Mon Sep 17 00:00:00 2001 From: nattthebear Date: Sat, 30 May 2020 11:12:31 -0400 Subject: [PATCH] Fix hex editor bulk multibyte reads The existing API was such that BulkPeekUint(Start: 20, EndInclusive: 39) would actually read from the address range [20, 100) instead of [20, 40). That's completely broken. The hex editor, the only consumer of the API, made the same exact mistake in reverse so the problem wasn't immediately apparent, but it was a giant land mine waiting to happen. Fix them both. --- .../tools/HexEditor/HexEditor.cs | 83 +++++++------------ .../Base Implementations/MemoryDomain.cs | 32 +++---- 2 files changed, 50 insertions(+), 65 deletions(-) diff --git a/src/BizHawk.Client.EmuHawk/tools/HexEditor/HexEditor.cs b/src/BizHawk.Client.EmuHawk/tools/HexEditor/HexEditor.cs index 09d3c797ce..8b744b3420 100644 --- a/src/BizHawk.Client.EmuHawk/tools/HexEditor/HexEditor.cs +++ b/src/BizHawk.Client.EmuHawk/tools/HexEditor/HexEditor.cs @@ -65,8 +65,6 @@ namespace BizHawk.Client.EmuHawk private MemoryDomain _domain = new NullMemoryDomain(); - private long _row; - private long _addr; private string _findStr = ""; private bool _mouseIsDown; private byte[] _rom; @@ -474,8 +472,8 @@ namespace BizHawk.Client.EmuHawk for (var i = 0; i < _rowsVisible; i++) { - _row = i + HexScrollBar.Value; - _addr = _row << 4; + long _row = i + HexScrollBar.Value; + long _addr = _row << 4; if (_addr >= _domain.Size) { break; @@ -503,8 +501,8 @@ namespace BizHawk.Client.EmuHawk var charValues = MakeValues(1); for (var i = 0; i < _rowsVisible; i++) { - _row = i + HexScrollBar.Value; - _addr = _row << 4; + long _row = i + HexScrollBar.Value; + long _addr = _row << 4; if (_addr >= _domain.Size) { break; @@ -550,63 +548,46 @@ namespace BizHawk.Client.EmuHawk private Dictionary MakeValues(int dataSize) { - var addresses = new List(); + long start = (long)HexScrollBar.Value << 4; + long end = (long)(HexScrollBar.Value + _rowsVisible) << 4; - for (var i = 0; i < _rowsVisible; i++) - { - _row = i + HexScrollBar.Value; - _addr = _row << 4; - if (_addr >= _domain.Size) - { - break; - } + end = Math.Min(end, _domain.Size); + end &= -(long)dataSize; - for (var j = 0; j < 16; j += dataSize) - { - if (_addr + j + dataSize <= _domain.Size) - { - var address = _addr + j; - addresses.Add(address); - } - } - } + var range = new MutableRange(start, end - 1); var dict = new Dictionary(); - var range = new MutableRange(addresses[0], addresses[0] + addresses.Count - 1); switch (dataSize) { default: case 1: { - var vals = new byte[addresses.Count]; - _domain.BulkPeekByte(range, vals); - for (var i = 0; i < addresses.Count; i++) - { - dict.Add(addresses[i], vals[i]); - } - break; - } + var vals = new byte[end - start]; + _domain.BulkPeekByte(range, vals); + int i = 0; + for (var addr = start; addr < end; addr += dataSize) + dict.Add(addr, vals[i++]); + break; + } case 2: - { - var vals = new ushort[addresses.Count]; - _domain.BulkPeekUshort(range, BigEndian, vals); - for (var i = 0; i < addresses.Count; i++) - { - dict.Add(addresses[i], vals[i]); - } - break; - } + { + var vals = new ushort[(end - start) >> 1]; + _domain.BulkPeekUshort(range, BigEndian, vals); + int i = 0; + for (var addr = start; addr < end; addr += dataSize) + dict.Add(addr, vals[i++]); + break; + } case 4: - { - var vals = new uint[addresses.Count]; - _domain.BulkPeekUint(range, BigEndian, vals); - for (var i = 0; i < addresses.Count; i++) - { - dict.Add(addresses[i], vals[i]); - } - break; - } + { + var vals = new uint[(end - start) >> 2]; + _domain.BulkPeekUint(range, BigEndian, vals); + int i = 0; + for (var addr = start; addr < end; addr += dataSize) + dict.Add(addr, vals[i++]); + break; + } } return dict; diff --git a/src/BizHawk.Emulation.Common/Base Implementations/MemoryDomain.cs b/src/BizHawk.Emulation.Common/Base Implementations/MemoryDomain.cs index 7d0558f9ad..5686d00408 100644 --- a/src/BizHawk.Emulation.Common/Base Implementations/MemoryDomain.cs +++ b/src/BizHawk.Emulation.Common/Base Implementations/MemoryDomain.cs @@ -125,18 +125,20 @@ namespace BizHawk.Emulation.Common { throw new ArgumentException(); } + var start = addresses.Start; + var end = addresses.EndInclusive + 1; - var nAddresses = (long) addresses.Count(); - if (nAddresses != values.Length) + if ((start & 1) != 0 || (end & 1) != 0) + throw new InvalidOperationException("The API contract doesn't define what to do for unaligned reads and writes!"); + + if (values.LongLength * 2 != end - start) { + // a longer array could be valid, but nothing needs that so don't support it for now throw new InvalidOperationException("Invalid length of values array"); } - var start = addresses.Start; - for (var i = 0; i < nAddresses; i++) - { - values[i] = PeekUshort(start + i*2, bigEndian); - } + for (var i = 0; i < values.Length; i++, start += 2) + values[i] = PeekUshort(start, bigEndian); } public virtual void BulkPeekUint(Range addresses, bool bigEndian, uint[] values) @@ -145,18 +147,20 @@ namespace BizHawk.Emulation.Common { throw new ArgumentException(); } + var start = addresses.Start; + var end = addresses.EndInclusive + 1; - var nAddresses = (long) addresses.Count(); - if (nAddresses != values.Length) + if ((start & 3) != 0 || (end & 3) != 0) + throw new InvalidOperationException("The API contract doesn't define what to do for unaligned reads and writes!"); + + if (values.LongLength * 4 != end - start) { + // a longer array could be valid, but nothing needs that so don't support it for now throw new InvalidOperationException("Invalid length of values array"); } - var start = addresses.Start; - for (var i = 0; i