[475] Avoid touching native memory from Structure.hashCode/equals

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
 ===========

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. */

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));
     }
 

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{ 

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() {