SecureRandom should be thread safe

Random is documented to be thread safe and our implementation appears
to be. However, the SecureRandom subclass implementation was not. This
was quite reproducible with the javax.net.ssl unit tests when run on a
multicore device, but not seen on a uniprocessor.

Details:

   Actual bug fix by adding synchronized to engine* methods

	luni/src/main/java/org/apache/harmony/security/provider/crypto/SHA1PRNG_SecureRandomImpl.java

   New testSecureRandomThreadSafety based on generateSeed errors in javax.net.ssl tests.

	luni/src/test/java/tests/security/SecureRandomTest.java

   Removing dalvik.annotation.* from assorted SecureRandom tests.

	luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandom2Test.java
	luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandomSpiTest.java
	luni/src/test/java/tests/java/security/SecureRandomTest.java
	luni/src/test/java/tests/security/SecureRandomTest.java

Change-Id: I5e8ece4c0836b02277d7cac1b9b2f60b3c4a7755

5 files changed, 45 insertions, 237 deletions

diff --git a/luni/src/main/java/org/apache/harmony/security/provider/crypto/SHA1PRNG_SecureRandomImpl.java b/luni/src/main/java/org/apache/harmony/security/provider/crypto/SHA1PRNG_SecureRandomImpl.java
index 1ffd55a..b99bd0f 100644
--- a/luni/src/main/java/org/apache/harmony/security/provider/crypto/SHA1PRNG_SecureRandomImpl.java
+++ b/luni/src/main/java/org/apache/harmony/security/provider/crypto/SHA1PRNG_SecureRandomImpl.java
@@ -198,7 +198,7 @@ public class SHA1PRNG_SecureRandomImpl extends SecureRandomSpi implements Serial
      * @throws
      *       NullPointerException - if null is passed to the "seed" argument
      */
-    protected void engineSetSeed(byte[] seed) {
+    protected synchronized void engineSetSeed(byte[] seed) {
 
         if (seed == null) {
             throw new NullPointerException("seed == null");
@@ -227,7 +227,7 @@ public class SHA1PRNG_SecureRandomImpl extends SecureRandomSpi implements Serial
      * @throws
      *       InvalidParameterException - if numBytes < 0
      */
-    protected byte[] engineGenerateSeed(int numBytes) {
+    protected synchronized byte[] engineGenerateSeed(int numBytes) {
 
         byte[] myBytes; // byte[] for bytes returned by "nextBytes()"
 
@@ -265,7 +265,7 @@ public class SHA1PRNG_SecureRandomImpl extends SecureRandomSpi implements Serial
      * @throws
      *       NullPointerException - if null is passed to the "bytes" argument
      */
-    protected void engineNextBytes(byte[] bytes) {
+    protected synchronized void engineNextBytes(byte[] bytes) {
 
         int i, n;
 
diff --git a/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandom2Test.java b/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandom2Test.java
index 88cc015..cf030c7 100644
--- a/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandom2Test.java
+++ b/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandom2Test.java
@@ -17,20 +17,14 @@
 
 package org.apache.harmony.security.tests.java.security;
 
-import dalvik.annotation.TestTargetClass;
-import dalvik.annotation.TestTargetNew;
-import dalvik.annotation.TestTargets;
-import dalvik.annotation.TestLevel;
-
 import java.security.NoSuchAlgorithmException;
 import java.security.NoSuchProviderException;
 import java.security.Provider;
 import java.security.SecureRandom;
 import java.security.SecureRandomSpi;
 import java.security.Security;
-
 import junit.framework.TestCase;
-@TestTargetClass(SecureRandom.class)
+
 public class SecureRandom2Test extends TestCase {
 
     private static final byte[] SEED_BYTES = { (byte) 33, (byte) 15, (byte) -3,
@@ -40,12 +34,6 @@ public class SecureRandom2Test extends TestCase {
 
     private static final long SEED_VALUE = 5335486759L;
 
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "getProvider",
-        args = {}
-    )
     public void testGetProvider() {
         SecureRandom sr1 = new SecureRandom();
         assertNotNull(sr1.getProvider());
@@ -66,14 +54,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#SecureRandom()
+     * java.security.SecureRandom#SecureRandom()
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "SecureRandom",
-        args = {}
-    )
     public void test_Constructor() {
         // Test for method java.security.SecureRandom()
         try {
@@ -84,14 +66,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#SecureRandom(byte[])
+     * java.security.SecureRandom#SecureRandom(byte[])
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "SecureRandom",
-        args = {byte[].class}
-    )
     public void test_Constructor$B() {
         // Test for method java.security.SecureRandom(byte [])
         try {
@@ -109,15 +85,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#SecureRandom(java.security.SecureRandomSpi,
-     *        java.security.Provider)
+     * java.security.SecureRandom#SecureRandom(java.security.SecureRandomSpi, java.security.Provider)
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "SecureRandom",
-        args = {java.security.SecureRandomSpi.class, java.security.Provider.class}
-    )
     public void test_ConstructorLjava_security_SecureRandomSpi_java_security_Provider() {
         try {
             new MySecureRandom(null, null);
@@ -140,14 +109,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#generateSeed(int)
+     * java.security.SecureRandom#generateSeed(int)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "",
-        method = "generateSeed",
-        args = {int.class}
-    )
     public void test_generateSeedI() {
         // Test for method byte [] java.security.SecureRandom.generateSeed(int)
         byte[] seed = new SecureRandom().generateSeed(SEED_SIZE);
@@ -162,14 +125,8 @@ public class SecureRandom2Test extends TestCase {
 
     }
     /**
-     * @tests java.security.SecureRandom#getInstance(java.lang.String)
+     * java.security.SecureRandom#getInstance(java.lang.String)
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "getInstance",
-        args = {java.lang.String.class}
-    )
     public void test_getInstanceLjava_lang_String() {
         // Test for method java.security.SecureRandom
         // java.security.SecureRandom.getInstance(java.lang.String)
@@ -188,15 +145,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#getInstance(java.lang.String,
-     *        java.lang.String)
+     * java.security.SecureRandom#getInstance(java.lang.String, java.lang.String)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL,
-        notes = "NoSuchAlgorithmException, NoSuchProviderException, IllegalArgumentException checking missed",
-        method = "getInstance",
-        args = {java.lang.String.class, java.lang.String.class}
-    )
     public void test_getInstanceLjava_lang_StringLjava_lang_String() {
         // Test for method java.security.SecureRandom
         // java.security.SecureRandom.getInstance(java.lang.String,
@@ -220,14 +170,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#getSeed(int)
+     * java.security.SecureRandom#getSeed(int)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Verification of negative parameter missed",
-        method = "getSeed",
-        args = {int.class}
-    )
     public void test_getSeedI() {
         // Test for method byte [] java.security.SecureRandom.getSeed(int)
         byte[] seed = SecureRandom.getSeed(SEED_SIZE);
@@ -242,14 +186,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#nextBytes(byte[])
+     * java.security.SecureRandom#nextBytes(byte[])
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Null array checking missed",
-        method = "nextBytes",
-        args = {byte[].class}
-    )
     public void test_nextBytes$B() {
         // Test for method void java.security.SecureRandom.nextBytes(byte [])
         byte[] bytes = new byte[313];
@@ -268,14 +206,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#setSeed(byte[])
+     * java.security.SecureRandom#setSeed(byte[])
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Null array checking missed",
-        method = "setSeed",
-        args = {byte[].class}
-    )
     public void test_setSeed$B() {
         // Test for method void java.security.SecureRandom.setSeed(byte [])
         try {
@@ -293,14 +225,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#setSeed(long)
+     * java.security.SecureRandom#setSeed(long)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "",
-        method = "setSeed",
-        args = {long.class}
-    )
     public void test_setSeedJ() {
         // Test for method void java.security.SecureRandom.setSeed(long)
         try {
@@ -317,14 +243,8 @@ public class SecureRandom2Test extends TestCase {
     }
 
     /**
-     * @tests java.security.SecureRandom#getAlgorithm()
+     * java.security.SecureRandom#getAlgorithm()
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "getAlgorithm",
-        args = {}
-    )
     public void test_getAlgorithm() {
         // Regression for HARMONY-750
 
@@ -350,12 +270,6 @@ public class SecureRandom2Test extends TestCase {
     }
 
     // Regression Test for HARMONY-3552.
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "next",
-        args = {int.class}
-    )
     public void test_nextJ() throws Exception {
         MySecureRandom mySecureRandom = new MySecureRandom(
                 new MySecureRandomSpi(), null);
diff --git a/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandomSpiTest.java b/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandomSpiTest.java
index 4f04de7..dd0f3d0 100644
--- a/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandomSpiTest.java
+++ b/luni/src/test/java/org/apache/harmony/security/tests/java/security/SecureRandomSpiTest.java
@@ -17,53 +17,19 @@
 
 package org.apache.harmony.security.tests.java.security;
 
-import dalvik.annotation.TestTargetClass;
-import dalvik.annotation.TestTargets;
-import dalvik.annotation.TestLevel;
-import dalvik.annotation.TestTargetNew;
-
 import java.security.SecureRandomSpi;
-
 import junit.framework.TestCase;
 
 /**
  * Tests for <code>SecureRandomSpi</code> class constructors
  * and methods.
- *
  */
-@TestTargetClass(SecureRandomSpi.class)
 public class SecureRandomSpiTest extends TestCase {
 
     /**
      * Test for <code>SecureRandomSpi</code> constructor
      * Assertion: constructs SecureRandomSpi
      */
-    @TestTargets({
-        @TestTargetNew(
-            level = TestLevel.COMPLETE,
-            notes = "",
-            method = "SecureRandomSpi",
-            args = {}
-        ),
-        @TestTargetNew(
-            level = TestLevel.COMPLETE,
-            notes = "",
-            method = "engineGenerateSeed",
-            args = {int.class}
-        ),
-        @TestTargetNew(
-            level = TestLevel.COMPLETE,
-            notes = "",
-            method = "engineNextBytes",
-            args = {byte[].class}
-        ),
-        @TestTargetNew(
-            level = TestLevel.COMPLETE,
-            notes = "",
-            method = "engineSetSeed",
-            args = {byte[].class}
-        )
-    })
     public void testSecureRandomSpi() {
         try {
             MySecureRandomSpi srs = new MySecureRandomSpi();
@@ -89,4 +55,4 @@ public class SecureRandomSpiTest extends TestCase {
             return null;
         }
     }
-}
\ No newline at end of file
+}
diff --git a/luni/src/test/java/tests/java/security/SecureRandomTest.java b/luni/src/test/java/tests/java/security/SecureRandomTest.java
index 2620175..b44280f 100644
--- a/luni/src/test/java/tests/java/security/SecureRandomTest.java
+++ b/luni/src/test/java/tests/java/security/SecureRandomTest.java
@@ -22,21 +22,14 @@
 
 package tests.java.security;
 
-import dalvik.annotation.TestTargetClass;
-import dalvik.annotation.TestTargets;
-import dalvik.annotation.TestLevel;
-import dalvik.annotation.TestTargetNew;
-
 import java.security.NoSuchAlgorithmException;
 import java.security.NoSuchProviderException;
 import java.security.Provider;
 import java.security.SecureRandom;
 import java.security.Security;
-
+import junit.framework.TestCase;
 import org.apache.harmony.security.tests.support.RandomImpl;
 
-import junit.framework.TestCase;
-@TestTargetClass(SecureRandom.class)
 /**
  * Tests for <code>SecureRandom</code> constructor and methods
  *
@@ -65,12 +58,6 @@ public class SecureRandomTest extends TestCase {
         Security.removeProvider(p.getName());
     }
 
-    @TestTargetNew(
-        level = TestLevel.PARTIAL,
-        notes = "Verification of negative and boundary parameters missed",
-        method = "next",
-        args = {int.class}
-    )
     public final void testNext() {
         MySecureRandom sr = new MySecureRandom();
         if (sr.nextElement(1) != 1 || sr.nextElement(2) != 3 || sr.nextElement(3) != 7) {
@@ -81,12 +68,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for void setSeed(long)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "",
-        method = "setSeed",
-        args = {long.class}
-    )
     public final void testSetSeedlong() {
         SecureRandom sr = new SecureRandom();
         sr.setSeed(12345);
@@ -95,12 +76,6 @@ public class SecureRandomTest extends TestCase {
         }
     }
 
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "",
-        method = "nextBytes",
-        args = {byte[].class}
-    )
     public final void testNextBytes() {
         byte[] b = new byte[5];
         SecureRandom sr = new SecureRandom();
@@ -122,12 +97,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for void SecureRandom()
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "SecureRandom",
-        args = {}
-    )
     public final void testSecureRandom() {
         SecureRandom sr = new SecureRandom();
         if (!sr.getAlgorithm().equals("someRandom")  ||
@@ -139,12 +108,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for void SecureRandom(byte[])
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL,
-        notes = "Null parameter checking missed",
-        method = "SecureRandom",
-        args = {byte[].class}
-    )
     public final void testSecureRandombyteArray() {
         byte[] b = {1,2,3};
         new SecureRandom(b);
@@ -159,12 +122,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for SecureRandom getInstance(String)
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL,
-        notes = "NoSuchAlgorithmException checking missed",
-        method = "getInstance",
-        args = {java.lang.String.class}
-    )
     public final void testGetInstanceString() {
         SecureRandom sr = null;
         try {
@@ -180,12 +137,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for SecureRandom getInstance(String, String)
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "getInstance",
-        args = {java.lang.String.class, java.lang.String.class}
-    )
     public final void testGetInstanceStringString() throws Exception {
         SecureRandom sr = SecureRandom.getInstance("someRandom", "SRProvider");
         if (sr.getProvider() != p || !"someRandom".equals(sr.getAlgorithm())) {
@@ -248,12 +199,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for SecureRandom getInstance(String, Provider)
      */
-    @TestTargetNew(
-        level = TestLevel.COMPLETE,
-        notes = "",
-        method = "getInstance",
-        args = {java.lang.String.class, java.security.Provider.class}
-    )
     public final void testGetInstanceStringProvider() throws Exception {
         Provider p = new SRProvider();
         SecureRandom sr = SecureRandom.getInstance("someRandom", p);
@@ -301,12 +246,6 @@ public class SecureRandomTest extends TestCase {
     /*
      * Class under test for void setSeed(byte[])
      */
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Verification with null parameter missed",
-        method = "setSeed",
-        args = {byte[].class}
-    )
     public final void testSetSeedbyteArray() {
         byte[] b = {1,2,3};
         SecureRandom sr = new SecureRandom();
@@ -317,12 +256,6 @@ public class SecureRandomTest extends TestCase {
 
     }
 
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Verification with invalid parameter missed",
-        method = "getSeed",
-        args = {int.class}
-    )
     public final void testGetSeed() {
         byte[] b = SecureRandom.getSeed(4);
         if( b.length != 4) {
@@ -330,12 +263,6 @@ public class SecureRandomTest extends TestCase {
         }
     }
 
-    @TestTargetNew(
-        level = TestLevel.PARTIAL_COMPLETE,
-        notes = "Verification with invalid parameter missed",
-        method = "generateSeed",
-        args = {int.class}
-    )
     public final void testGenerateSeed() {
         SecureRandom sr = new SecureRandom();
         byte[] b = sr.generateSeed(4);
diff --git a/luni/src/test/java/tests/security/SecureRandomTest.java b/luni/src/test/java/tests/security/SecureRandomTest.java
index cf9c6dc..bb038e1 100644
--- a/luni/src/test/java/tests/security/SecureRandomTest.java
+++ b/luni/src/test/java/tests/security/SecureRandomTest.java
@@ -15,19 +15,20 @@
  */
 package tests.security;
 
-import dalvik.annotation.TestLevel;
-import dalvik.annotation.TestTargetNew;
-import dalvik.annotation.TestTargets;
 import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
 import java.util.Arrays;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorCompletionService;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import junit.framework.TestCase;
-public abstract class SecureRandomTest extends TestCase {
 
+public abstract class SecureRandomTest extends TestCase {
 
     private final String algorithmName;
 
-    private int counter=0;
+    private int counter = 0;
 
     protected SecureRandomTest(String name) {
         this.algorithmName = name;
@@ -37,28 +38,6 @@ public abstract class SecureRandomTest extends TestCase {
         super.setUp();
     }
 
-    @TestTargets({
-        @TestTargetNew(
-                level=TestLevel.ADDITIONAL,
-                method="getInstance",
-                args={String.class}
-        ),
-        @TestTargetNew(
-                level=TestLevel.ADDITIONAL,
-                method="setSeed",
-                args={long.class}
-        ),
-        @TestTargetNew(
-                level=TestLevel.ADDITIONAL,
-                method="nextBytes",
-                args={byte[].class}
-        ),
-        @TestTargetNew(
-                level=TestLevel.COMPLETE,
-                method="method",
-                args={}
-        )
-    })
     public void testSecureRandom() {
         SecureRandom secureRandom1 = null;
         try {
@@ -92,4 +71,26 @@ public abstract class SecureRandomTest extends TestCase {
 
         return randomData;
     }
+
+
+    public void testSecureRandomThreadSafety() throws Exception {
+        final SecureRandom secureRandom = SecureRandom.getInstance(algorithmName);
+        int threads = 2;
+        ExecutorService executor = Executors.newFixedThreadPool(threads);
+        ExecutorCompletionService ecs = new ExecutorCompletionService(executor);
+        for (int t = 0; t < threads; t++) {
+            ecs.submit(new Callable<Void>() {
+                public Void call () {
+                    for (int i = 0; i < 1000; i++) {
+                        secureRandom.generateSeed(1024);
+                    }
+                    return null;
+                }
+            });
+        }
+        executor.shutdown();
+        for (int i = 0; i < threads; i++) {
+            ecs.take().get();
+        }
+    }
 }