[CODEC-264]: Ensure hash128 maintains the sign extension bug.

aherbert · aherbert · commit f5a61f0cd029 · 2020-01-18T01:59:55.000Z
The hash128(...) method was calling the public hash128x86(...) method
with an int seed when it should have called the private hash128x86(...)
with a long seed. Thus it did not have the sign extension error. The
internal method has been renamed to avoid a name clash with the public
API. The old hash128() behaviour is now restored as the seed is passed
to the hash method with implicit long conversion.
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
@@ -42,6 +42,10 @@ The <action> type attribute can be add,update,fix,remove.
   </properties>
   <body>
 
+    <release version="1.15" date="YYYY-MM-DD" description="Feature and fix release.">
+      <action issue="CODEC-264" dev="aherbert" due-to="Andy Seaborne" type="fix">MurmurHash3: Ensure hash128 maintains the sign extension bug.</action>
+    </release>
+
     <release version="1.14" date="2019-12-30" description="Feature and fix release.">
       <action issue="CODEC-261" dev="aherbert" type="fix">Hex: Allow encoding read-only ByteBuffer.</action>
       <action issue="CODEC-259" dev="aherbert" type="fix">Hex: Only use an available ByteBuffer backing array if the length equals the remaining byte count.</action>
diff --git a/src/main/java/org/apache/commons/codec/digest/MurmurHash3.java b/src/main/java/org/apache/commons/codec/digest/MurmurHash3.java
@@ -800,9 +800,12 @@ public static long[] hash128(final String data) {
     @Deprecated
     public static long[] hash128(final byte[] data, final int offset, final int length, final int seed) {
         // ************
-        // Note: This fails to apply masking using 0xffffffffL to the seed.
+        // Note: This deliberately fails to apply masking using 0xffffffffL to the seed
+        // to maintain behavioural compatibility with the original version.
+        // The implicit conversion to a long will extend a negative sign
+        // bit through the upper 32-bits of the long seed. These should be zero.
         // ************
-        return hash128x64(data, offset, length, seed);
+        return hash128x64Internal(data, offset, length, seed);
     }
 
     /**
@@ -820,7 +823,7 @@ public static long[] hash128(final byte[] data, final int offset, final int leng
      */
     public static long[] hash128x64(final byte[] data, final int offset, final int length, final int seed) {
         // Use an unsigned 32-bit integer as the seed
-        return hash128x64(data, offset, length, seed & 0xffffffffL);
+        return hash128x64Internal(data, offset, length, seed & 0xffffffffL);
     }
 
     /**
@@ -835,7 +838,7 @@ public static long[] hash128x64(final byte[] data, final int offset, final int l
      * @param seed The initial seed value
      * @return The 128-bit hash (2 longs)
      */
-    private static long[] hash128x64(final byte[] data, final int offset, final int length, final long seed) {
+    private static long[] hash128x64Internal(final byte[] data, final int offset, final int length, final long seed) {
         long h1 = seed;
         long h2 = seed;
         final int nblocks = length >> 4;
diff --git a/src/test/java/org/apache/commons/codec/digest/MurmurHash3Test.java b/src/test/java/org/apache/commons/codec/digest/MurmurHash3Test.java
@@ -559,7 +559,7 @@ public void testHash128() {
             {1237530251176898868L, 6144786892208594932L}, {2347717913548230384L, -7461066668225718223L},
             {-7963311463560798404L, 8435801462986138227L}, {-7493166089060196513L, 8163503673197886404L},
             {6807249306539951962L, -1438886581269648819L}, {6752656991043418179L, 6334147827922066123L},
-            {-4534351735605790331L, -4530801663887858236L}, {-7886946241830957955L, -6261339648449285315L},        };
+            {-4534351735605790331L, -4530801663887858236L}, {-7886946241830957955L, -6261339648449285315L},};
         for (int i = 0; i < answers.length; i++) {
             final byte[] bytes = Arrays.copyOf(RANDOM_BYTES, i);
             Assert.assertArrayEquals(answers[i], MurmurHash3.hash128(bytes));
@@ -605,6 +605,43 @@ public void testHash128WithOffsetLengthAndSeed() {
         }
     }
 
+    /**
+     * Test the {@link MurmurHash3#hash128(byte[], int, int, int)} algorithm.
+     *
+     * <p>Explicit test for a negative seed. The original implementation has a sign extension error
+     * for negative seeds. This test is here to maintain behavioural compatibility of the
+     * broken deprecated method.
+     */
+    @Test
+    public void testHash128WithOffsetLengthAndNegativeSeed() {
+        // Seed can be negative
+        final int seed = -42;
+        final int offset = 13;
+
+        // Test with all sizes up to 31 bytes. This ensures a full round of 16-bytes plus up to
+        // 15 bytes remaining.
+        final long[][] answers = {{5954234972212089025L, 3342108296337967352L},
+                {8501094764898402923L, 7873951092908129427L}, {-3334322685492296196L, -2842715922956549478L},
+                {-2177918982459519644L, -1612349961980368636L}, {4172870320608886992L, -4177375712254136503L},
+                {7546965006884307324L, -5222114032564054641L}, {-2885083166621537267L, -2069868899915344482L},
+                {-2397098497873118851L, 4990578036999888806L}, {-706479374719025018L, 7531201699171849870L},
+                {6487943141157228609L, 3576221902299447884L}, {6671331646806999453L, -3428049860825046360L},
+                {-8700221138601237020L, -2748450904559980545L}, {-9028762509863648063L, 6130259301750313402L},
+                {729958512305702590L, -36367317333638988L}, {-3803232241584178983L, -4257744207892929651L},
+                {5734013720237474696L, -760784490666078990L}, {-6097477411153026656L, 625288777006549065L},
+                {1320365359996757504L, -2251971390373072541L}, {5551441703887653022L, -3516892619809375941L},
+                {698875391638415033L, 3456972931370496131L}, {5874956830271303805L, -6074126509360777023L},
+                {-7030758398537734781L, -3174643657101295554L}, {6835789852786226556L, 7245353136839389595L},
+                {-7755767580598793204L, -6680491060945077989L}, {-3099789923710523185L, -2751080516924952518L},
+                {-7772046549951435453L, 5263206145535830491L}, {7458715941971015543L, 5470582752171544854L},
+                {-7753394773760064468L, -2330157750295630617L}, {-5899278942232791979L, 6235686401271389982L},
+                {4881732293467626532L, 2617335658565007304L}, {-5722863941703478257L, -5424475653939430258L},
+                {-3703319768293496315L, -2124426428486426443L},};
+        for (int i = 0; i < answers.length; i++) {
+            Assert.assertArrayEquals("Length: " + i, answers[i], MurmurHash3.hash128(RANDOM_BYTES, offset, i, seed));
+        }
+    }
+
     /**
      * Test the {@link MurmurHash3#hash128(String)} algorithm. This only tests it can return
      * the same value as {@link MurmurHash3#hash128(byte[], int, int, int)} if the string
