From 220c2476c81b2fa42a1e7d7b8cb53a99b75ba72e Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Wed, 1 May 2024 23:28:59 +0200 Subject: [PATCH] Allow address 0 when bytesCount is 0 for `copyMemory` and `setMemory` Resolves #3 --- .../agent_impl/MemoryTracker.java | 17 ++- .../agent_impl/UnsafeSanitizerImpl.java | 7 ++ .../UnsafeThrowErrorTest.java | 108 +++++++++++++++++- 3 files changed, 124 insertions(+), 8 deletions(-) diff --git a/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/MemoryTracker.java b/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/MemoryTracker.java index fd800f1..dc2201a 100644 --- a/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/MemoryTracker.java +++ b/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/MemoryTracker.java @@ -230,6 +230,17 @@ public boolean onAccess(long address, long bytesCount, boolean isRead) { } public boolean onCopy(long srcAddress, long destAddress, long bytesCount) { + if (bytesCount == 0) { + // Allow address to be right behind section ('exclusive end address') since copy size is 0 + boolean isZeroSized = true; + + // Allow address 0; this assumes that user code might first call `Unsafe#allocateMemory` and then uses + // the returned address as argument to `copyMemory`; + // if the bytes count is 0, then `allocateMemory` will return 0 as address + return (srcAddress == 0 || verifyValidAddress(srcAddress, isZeroSized)) + && (destAddress == 0 || verifyValidAddress(destAddress, isZeroSized)); + } + if (srcAddress <= 0) { return reportError("Invalid srcAddress: " + srcAddress); } else if (destAddress <= 0) { @@ -238,12 +249,6 @@ public boolean onCopy(long srcAddress, long destAddress, long bytesCount) { return false; } - if (bytesCount == 0) { - // Allow address to be right behind section ('exclusive end address') since copy size is 0 - boolean isZeroSized = true; - return verifyValidAddress(srcAddress, isZeroSized) && verifyValidAddress(destAddress, isZeroSized); - } - lock.writeLock().lock(); try { sectionsMap.performCopyAccess(srcAddress, destAddress, bytesCount); diff --git a/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/UnsafeSanitizerImpl.java b/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/UnsafeSanitizerImpl.java index 8ac9413..bb37f7d 100644 --- a/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/UnsafeSanitizerImpl.java +++ b/agent-impl/src/main/java/marcono1234/unsafe_sanitizer/agent_impl/UnsafeSanitizerImpl.java @@ -357,6 +357,13 @@ public static boolean onWriteAccess(@Nullable Object obj, long address, long byt private static boolean onAccess(@Nullable Object obj, long address, long bytesCount, boolean isRead) { if (obj == null) { + // If `bytesCount` is 0, allow address 0; this assumes that user code might first call `Unsafe#allocateMemory` + // and then uses the returned address as argument to `copyMemory` or `setMemory`; + // if the bytes count is 0, then `allocateMemory` will return 0 as address + if (bytesCount == 0 && address == 0) { + return true; + } + var memoryTracker = getMemoryTracker(); return memoryTracker == null || memoryTracker.onAccess(address, bytesCount, isRead); diff --git a/src/test/java/marcono1234/unsafe_sanitizer/UnsafeThrowErrorTest.java b/src/test/java/marcono1234/unsafe_sanitizer/UnsafeThrowErrorTest.java index 074420a..0bbd1fc 100644 --- a/src/test/java/marcono1234/unsafe_sanitizer/UnsafeThrowErrorTest.java +++ b/src/test/java/marcono1234/unsafe_sanitizer/UnsafeThrowErrorTest.java @@ -567,6 +567,29 @@ void copyMemoryArray() { unsafe.copyMemory(a, from, a, to, 2 * Long.BYTES); assertArrayEquals(new long[] {1, 2, 1, 2}, a); }); + + assertNoBadMemoryAccess(() -> { + byte[] a = {1}; + byte[] b = {0}; + // Copy of size 0 should have no effect + unsafe.copyMemory(a, ARRAY_BYTE_BASE_OFFSET, b, ARRAY_BYTE_BASE_OFFSET, 0); + assertArrayEquals(new byte[] {1}, a); + assertArrayEquals(new byte[] {0}, b); + + // Copy of size 0 should also be allowed right behind last array element, i.e. at 'exclusive end address' (but should have no effect) + long endOffset = ARRAY_BYTE_BASE_OFFSET + a.length * ARRAY_BYTE_INDEX_SCALE; + unsafe.copyMemory(a, endOffset, b, ARRAY_BYTE_BASE_OFFSET, 0); + unsafe.copyMemory(b, ARRAY_BYTE_BASE_OFFSET, a, endOffset, 0); + assertArrayEquals(new byte[] {1}, a); + assertArrayEquals(new byte[] {0}, b); + + // Size 0 should allow using address 0 (should have no effect) + unsafe.copyMemory(a, ARRAY_BYTE_BASE_OFFSET, null, 0, 0); + unsafe.copyMemory(null, 0, b, ARRAY_BYTE_BASE_OFFSET, 0); + assertArrayEquals(new byte[] {1}, a); + assertArrayEquals(new byte[] {0}, b); + }); + assertBadMemoryAccess(() -> { byte[] b = {1, 2, 3, 4}; long from = ARRAY_BYTE_BASE_OFFSET; @@ -575,6 +598,28 @@ void copyMemoryArray() { unsafe.copyMemory(b, from, b, to, 2); }); + { + byte[] a = {1}; + // Should fail for invalid address, even if size is 0 + long maxOffset = ARRAY_BYTE_BASE_OFFSET + a.length * ARRAY_BYTE_INDEX_SCALE; + var e = assertBadMemoryAccess(() -> unsafe.copyMemory(a, ARRAY_BYTE_BASE_OFFSET, a, maxOffset + 1, 0)); + assertEquals("Bad array access at offset " + (maxOffset + 1) + ", size 0; max offset is " + maxOffset, e.getMessage()); + e = assertBadMemoryAccess(() -> unsafe.copyMemory(a, ARRAY_BYTE_BASE_OFFSET, null, -1, 0)); + assertEquals("Invalid address: -1", e.getMessage()); + e = assertBadMemoryAccess(() -> unsafe.copyMemory(null, -1, a, ARRAY_BYTE_BASE_OFFSET, 0)); + assertEquals("Invalid address: -1", e.getMessage()); + } + + { + byte[] b = {1}; + // Should fail for array offset 0 (assuming that base offset != 0), even if size is 0 + assert ARRAY_BYTE_BASE_OFFSET > 0; + var e = assertBadMemoryAccess(() -> unsafe.copyMemory(b, 0, b, ARRAY_BYTE_BASE_OFFSET, 0)); + assertEquals("Bad array access at offset 0; min offset is " + ARRAY_BYTE_BASE_OFFSET, e.getMessage()); + e = assertBadMemoryAccess(() -> unsafe.copyMemory(b, ARRAY_BYTE_BASE_OFFSET, b, 0, 0)); + assertEquals("Bad array access at offset 0; min offset is " + ARRAY_BYTE_BASE_OFFSET, e.getMessage()); + } + var e = assertBadMemoryAccess(() -> { Object[] a = {"a", "b"}; // `copyMemory` only permits primitive arrays @@ -603,6 +648,18 @@ public String toString() { dummy.a = 1; var e = assertBadMemoryAccess(() -> unsafe.copyMemory(dummy, offsetA, dummy, offsetB, 2)); assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); + + // Should fail even if size is 0 + e = assertBadMemoryAccess(() -> unsafe.copyMemory(dummy, offsetA, dummy, offsetB, 0)); + assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); + + // Should fail for offset 0 (assuming that no field is at that offset), even if size is 0 + assert offsetA != 0 && offsetB != 0; + byte[] b = {1}; + e = assertBadMemoryAccess(() -> unsafe.copyMemory(dummy, 0, b, ARRAY_BYTE_BASE_OFFSET, 0)); + assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); + e = assertBadMemoryAccess(() -> unsafe.copyMemory(b, ARRAY_BYTE_BASE_OFFSET, dummy, 0, 0)); + assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); } @Test @@ -621,11 +678,18 @@ void copyMemory() { assertEquals(longValue, unsafe.getLong(a)); assertEquals(0, unsafe.getLong(b)); - // Copy of size 0 should also be allowed right behind sections, i.e. at 'exclusive end address' + // Copy of size 0 should also be allowed right behind sections, i.e. at 'exclusive end address' (but should have no effect) unsafe.copyMemory(null, a + size, null, b + size, 0); assertEquals(longValue, unsafe.getLong(a)); assertEquals(0, unsafe.getLong(b)); + // Size 0 should allow using address 0 (should have no effect) + unsafe.copyMemory(null, 0, null, 0, 0); + unsafe.copyMemory(null, a, null, 0, 0); + unsafe.copyMemory(null, 0, null, b, 0); + assertEquals(longValue, unsafe.getLong(a)); + assertEquals(0, unsafe.getLong(b)); + unsafe.copyMemory(null, a, null, b, size); assertEquals(longValue, unsafe.getLong(a)); assertEquals(longValue, unsafe.getLong(b)); @@ -665,6 +729,12 @@ void copyMemory() { assertBadMemoryAccess(() -> unsafe.copyMemory(null, a, null, b + 1, size)); assertBadMemoryAccess(() -> unsafe.copyMemory(a, b + 1, size)); + // Should fail for invalid address, even if size is 0 + var e = assertBadMemoryAccess(() -> unsafe.copyMemory(-1, 0, 0)); + assertEquals("Invalid address: -1", e.getMessage()); + e = assertBadMemoryAccess(() -> unsafe.copyMemory(0, -1, 0)); + assertEquals("Invalid address: -1", e.getMessage()); + assertBadMemoryAccess(() -> { class Dummy { int i; @@ -718,6 +788,17 @@ void setMemoryArray() { unsafe.setMemory(a, ARRAY_OBJECT_BASE_OFFSET, ARRAY_OBJECT_INDEX_SCALE, (byte) 2); }); assertEquals("Unsupported class " + Object[].class.getTypeName(), e.getMessage()); + + e = assertBadMemoryAccess(() -> { + byte[] b = {1, 2, 3, 4}; + // Should fail for array offset 0 (assuming that base offset != 0), even if size is 0 + assert ARRAY_BYTE_BASE_OFFSET > 0; + unsafe.setMemory(b, 0, 0, (byte) 5); + }); + assertEquals( + "Bad array access at offset 0; min offset is " + ARRAY_BYTE_BASE_OFFSET, + e.getMessage() + ); } @Test @@ -737,6 +818,15 @@ public String toString() { dummy.a = 1; var e = assertBadMemoryAccess(() -> unsafe.setMemory(dummy, offset, 4, (byte) 2)); assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); + + // Should fail even if size is 0 + e = assertBadMemoryAccess(() -> unsafe.setMemory(dummy, offset, 0, (byte) 2)); + assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); + + // Should fail for offset 0 (assuming that no field is at that offset), even if size is 0 + assert offset != 0; + e = assertBadMemoryAccess(() -> unsafe.setMemory(dummy, 0, 0, (byte) 2)); + assertEquals("Unsupported class " + Dummy.class.getTypeName(), e.getMessage()); } @Test @@ -750,10 +840,13 @@ void setMemory() { unsafe.setMemory(a, 0, (byte) 1); assertEquals(0, unsafe.getLong(a)); - // Size 0 should also be allowed right behind section, i.e. at 'exclusive end address' + // Size 0 should also be allowed right behind section, i.e. at 'exclusive end address' (but should have no effect) unsafe.setMemory(a + size, 0, (byte) 1); assertEquals(0, unsafe.getLong(a)); + // Size 0 should allow using address 0 (should have no effect) + unsafe.setMemory(0, 0, (byte) 1); + unsafe.setMemory(a + 2, 4, (byte) 0x12); assertEquals(0x12121212, unsafe.getInt(a + 2)); // Other memory should have remained unmodified @@ -767,6 +860,10 @@ void setMemory() { var e = assertBadMemoryAccess(() -> unsafe.setMemory(-1, 2, (byte) 0)); assertEquals("Invalid address: -1", e.getMessage()); + // Should fail for invalid address, even if size is 0 + e = assertBadMemoryAccess(() -> unsafe.setMemory(-1, 0, (byte) 0)); + assertEquals("Invalid address: -1", e.getMessage()); + long size = 8; long a = allocateMemory(size); @@ -779,6 +876,13 @@ void setMemory() { e.getMessage() ); + // Should fail for invalid address, even if size is 0 + e = assertBadMemoryAccess(() -> unsafe.setMemory(a + size + 1, 0, (byte) 4)); + assertEquals( + "Access outside of section at " + (a + size + 1) + ", size 0 (previous section: " + a + ", size 8)", + e.getMessage() + ); + // Clean up freeMemory(a); }