From ca585ca3d6daa183aabefb5079e9014e904d20e2 Mon Sep 17 00:00:00 2001 From: Timothy Wall Date: Sun, 30 Aug 2015 10:20:07 -0400 Subject: [PATCH] Fix issue #475 --- CHANGES.md | 1 + src/com/sun/jna/Structure.java | 53 ++++++++-------- test/com/sun/jna/ArgumentsMarshalTest.java | 4 +- test/com/sun/jna/CallbacksTest.java | 4 +- test/com/sun/jna/StructureTest.java | 73 +++++++--------------- 5 files changed, 51 insertions(+), 84 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 9dcb9d8432..df9c24cbc6 100755 --- a/CHANGES.md +++ b/CHANGES.md @@ -73,6 +73,7 @@ Bug Fixes * [#404](https://github.com/twall/jna/pull/404): Fix `VARIANT` constructors for `int`, `short`, and `long` - [@lwahonen](https://github.com/lwahonen). * [#420](https://github.com/twall/jna/pull/420): Fix structure leaving always one element in ThreadLocal set - [@sjappig](https://github.com/sjappig). * [#467](https://github.com/twall/jna/issues/467): Fix TypeMapper usage with direct-mapped libraries converting primitives to Java objects (specifically enums) - [@twall](https://github.com/twall). +* [#475](https://github.com/twall/jna/issues/475): Avoid modifying native memory in `Structure.equals()/hashCode()`- [@twall](https://github.com/twall). Release 4.1 =========== diff --git a/src/com/sun/jna/Structure.java b/src/com/sun/jna/Structure.java index d8525da173..288f48339a 100644 --- a/src/com/sun/jna/Structure.java +++ b/src/com/sun/jna/Structure.java @@ -1462,42 +1462,39 @@ private Class baseClass() { return getClass(); } - /** This structure is equal to another based on the same data type - * and memory contents. + /** Return whether the given Structure's backing data is identical to + * this one. */ - public boolean equals(Object o) { - if (o == this) { - return true; - } - if (!(o instanceof Structure)) { - return false; - } - if (o.getClass() != getClass() - && ((Structure)o).baseClass() != baseClass()) { - return false; - } - Structure s = (Structure)o; - if (s.getPointer().equals(getPointer())) { + public boolean dataEquals(Structure s) { + byte[] data = s.getPointer().getByteArray(0, s.size()); + byte[] ref = getPointer().getByteArray(0, size()); + if (data.length == ref.length) { + for (int i=0;i < data.length;i++) { + if (data[i] != ref[i]) { + System.out.println("byte mismatch at offset " + i); + return false; + } + } return true; } - if (s.size() == size()) { - clear(); write(); - byte[] buf = getPointer().getByteArray(0, size()); - s.clear(); s.write(); - byte[] sbuf = s.getPointer().getByteArray(0, s.size()); - return Arrays.equals(buf, sbuf); - } return false; } - /** Since {@link #equals} depends on the contents of memory, use that - * as the basis for the hash code. + /** Structures are equal if they share the same pointer. */ + public boolean equals(Object o) { + return o instanceof Structure + && o.getClass() == getClass() + && ((Structure)o).getPointer().equals(getPointer()); + } + + /** Use the underlying memory pointer's hash code. */ public int hashCode() { - clear(); write(); - Adler32 code = new Adler32(); - code.update(getPointer().getByteArray(0, size())); - return (int)code.getValue(); + Pointer p = getPointer(); + if (p != null) { + return getPointer().hashCode(); + } + return getClass().hashCode(); } /** Cache native type information for use in native code. */ diff --git a/test/com/sun/jna/ArgumentsMarshalTest.java b/test/com/sun/jna/ArgumentsMarshalTest.java index 018ae27d4a..9ba07f6a47 100644 --- a/test/com/sun/jna/ArgumentsMarshalTest.java +++ b/test/com/sun/jna/ArgumentsMarshalTest.java @@ -565,9 +565,9 @@ public void testStructureByReferenceArrayArgument() { null, new CheckFieldAlignment.ByReference(), }; - assertEquals("Wrong value returned (0)", args[0], lib.returnPointerArrayElement(args, 0)); + assertTrue("Wrong value returned (0)", args[0].dataEquals(lib.returnPointerArrayElement(args, 0))); assertNull("Wrong value returned (1)", lib.returnPointerArrayElement(args, 1)); - assertEquals("Wrong value returned (2)", args[2], lib.returnPointerArrayElement(args, 2)); + assertTrue("Wrong value returned (2)", args[2].dataEquals(lib.returnPointerArrayElement(args, 2))); assertNull("Native array should be null terminated", lib.returnPointerArrayElement(args, 3)); } diff --git a/test/com/sun/jna/CallbacksTest.java b/test/com/sun/jna/CallbacksTest.java index 3d59fcbf7f..f4d968df66 100644 --- a/test/com/sun/jna/CallbacksTest.java +++ b/test/com/sun/jna/CallbacksTest.java @@ -741,8 +741,8 @@ public TestStructure.ByValue callback(TestStructure.ByValue s) { + arg[0].getPointer(), arg[0].getPointer() instanceof Memory); assertTrue("ByValue result should own its own memory, instead was " + result.getPointer(), result.getPointer() instanceof Memory); - assertEquals("Wrong value for callback argument", s, arg[0]); - assertEquals("Wrong value for callback result", s, result); + assertTrue("Wrong value for callback argument", s.dataEquals(arg[0])); + assertTrue("Wrong value for callback result", s.dataEquals(result)); } public void testUnionByValueCallbackArgument() throws Exception{ diff --git a/test/com/sun/jna/StructureTest.java b/test/com/sun/jna/StructureTest.java index eec0296a4d..bc8064a5ca 100644 --- a/test/com/sun/jna/StructureTest.java +++ b/test/com/sun/jna/StructureTest.java @@ -895,6 +895,12 @@ protected List getFieldOrder() { assertEquals("Non-volatile field should be written", 1, s.getPointer().getInt(4)); s.writeField("counter"); assertEquals("Explicit volatile field write failed", 1, s.getPointer().getInt(0)); + + s.equals(s); + assertEquals("Structure equals should leave volatile field unchanged", 1, s.getPointer().getInt(0)); + + s.hashCode(); + assertEquals("Structure equals should leave volatile field unchanged", 1, s.getPointer().getInt(0)); } public static class StructureWithPointers extends Structure { @@ -1542,53 +1548,27 @@ class TestStructure extends Structure { protected List getFieldOrder() { return Arrays.asList(new String[] { "first", "second", "third" }); } + public TestStructure() { } + public TestStructure(Pointer p) { super(p); } } OtherStructure s0 = new OtherStructure(); TestStructure s1 = new TestStructure(); - TestStructure s2 = new TestStructure(); - TestStructure s3 = new TestStructure(); - int VALUE = 99; - s1.first = s2.first = s3.first = VALUE; + TestStructure s2 = new TestStructure(s1.getPointer()); + TestStructure s3 = new TestStructure(s2.getPointer()); + TestStructure s4 = new TestStructure(); assertFalse("Structures of different classes with same fields are not equal", s1.equals(s0)); assertFalse("Structures of different classes with same fields are not equal (reflexive)", s0.equals(s1)); assertFalse("Compare to null failed", s1.equals(null)); assertTrue("Equals is not reflexive", s1.equals(s1)); - assertTrue("Equals failed on identical structures", s1.equals(s2)); + assertTrue("Equals failed on structures with same pointer", s1.equals(s2)); assertTrue("Equals is not symmetric", s2.equals(s1)); assertTrue("Equals is not transitive", s1.equals(s2) && s2.equals(s3) && s1.equals(s3)); - - + assertFalse("Compare to different structure failed", s1.equals(s4)); } - public void testStructureEqualsByValueByReference() { - class TestStructure extends Structure { - public int first; - public int[] second = new int[4]; - public Pointer[] third = new Pointer[4]; - protected List getFieldOrder() { - return Arrays.asList(new String[] { "first", "second", "third" }); - } - } - class ByReference extends TestStructure implements Structure.ByReference { } - class ByValue extends TestStructure implements Structure.ByValue { } - TestStructure s1 = new TestStructure(); - TestStructure s2 = new ByReference(); - TestStructure s3 = new ByValue(); - int VALUE = 99; - s1.first = s2.first = s3.first = VALUE; - - assertTrue("Equals failed on identical ByReference", s1.equals(s2)); - assertTrue("Equals is not symmetric (ByReference)", s2.equals(s1)); - assertTrue("Equals failed on identical ByValue", s1.equals(s3)); - assertTrue("Equals is not symmetric (ByValue)", s3.equals(s1)); - assertTrue("Equals is not transitive (ByReference/ByValue)", s1.equals(s2) && s2.equals(s3) && s1.equals(s3)); - - - } - - public void testStructureHashCodeMatchesEqualsTrue() { + public void testStructureHashCodeMatchesWhenEqual() { class TestStructure extends Structure { public int first; protected List getFieldOrder() { @@ -1597,26 +1577,15 @@ protected List getFieldOrder() { } TestStructure s1 = new TestStructure(); TestStructure s2 = new TestStructure(); - s1.first = s2.first = 0x12345678; - assertEquals("hashCode should match when structures equal", - s1.hashCode(), s2.hashCode()); - } + assertFalse("hashCode should be different for two different structures", s1.hashCode() == s2.hashCode()); - public void testStructureEqualsIgnoresPadding() { - class TestStructure extends Structure { - public byte first; - public int second; - protected List getFieldOrder() { - return Arrays.asList(new String[] { "first", "second" }); - } - } - TestStructure s1 = new TestStructure(); - TestStructure s2 = new TestStructure(); + s2.useMemory(s1.getPointer()); + assertEquals("hashCode should match when structures have same pointer", + s1.hashCode(), s2.hashCode()); - // Make padding bits non-zero - s2.getPointer().setInt(0, -1); - s2.write(); - assertTrue("Structure equals should ignore padding", s1.equals(s2)); + s2.useMemory(s1.getPointer()); + assertEquals("hashCode should match when structures have same pointer", + s1.hashCode(), s2.hashCode()); } public void testRecursiveWrite() {