# HG changeset patch # User weijun # Date 1479387759 -28800 # Node ID 5b0b84715c069a93070c6aa05ce88119e944be2a # Parent c229da92b1a93b56cf52e511565ba52de0127a89 7004967: SecureRandom should be more explicit about threading 8169312: SecureRandom::getSeed(num) not specified if num is negative Reviewed-by: mullan, xuelei diff -r c229da92b1a9 -r 5b0b84715c06 jdk/src/java.base/share/classes/java/security/SecureRandom.java --- a/jdk/src/java.base/share/classes/java/security/SecureRandom.java Thu Nov 17 11:55:59 2016 +0000 +++ b/jdk/src/java.base/share/classes/java/security/SecureRandom.java Thu Nov 17 21:02:39 2016 +0800 @@ -120,6 +120,24 @@ * gathered, for example, if the entropy source is /dev/random on various * Unix-like operating systems. * + *

Thread safety

+ * {@code SecureRandom} objects are safe for use by multiple concurrent threads. + * + * @implSpec + * A {@code SecureRandom} service provider can advertise that it is thread-safe + * by setting the service + * provider attribute "ThreadSafe" to "true" when registering the provider. + * Otherwise, this class will instead synchronize access to the following + * methods of the {@code SecureRandomSpi} implementation: + * + * * @see java.security.SecureRandomSpi * @see java.util.Random * @@ -150,6 +168,14 @@ */ private SecureRandomSpi secureRandomSpi = null; + /** + * Thread safety. + * + * @serial + * @since 9 + */ + private final boolean threadSafe; + /* * The algorithm name of null if unknown. * @@ -189,6 +215,16 @@ */ super(0); getDefaultPRNG(false, null); + this.threadSafe = getThreadSafe(); + } + + private boolean getThreadSafe() { + if (provider == null || algorithm == null) { + return false; + } else { + return Boolean.parseBoolean(provider.getProperty( + "SecureRandom." + algorithm + " ThreadSafe", "false")); + } } /** @@ -217,6 +253,7 @@ public SecureRandom(byte[] seed) { super(0); getDefaultPRNG(true, seed); + this.threadSafe = getThreadSafe(); } private void getDefaultPRNG(boolean setSeed, byte[] seed) { @@ -269,6 +306,7 @@ this.secureRandomSpi = secureRandomSpi; this.provider = provider; this.algorithm = algorithm; + this.threadSafe = getThreadSafe(); if (!skipDebug && pdebug != null) { pdebug.println("SecureRandom." + algorithm + @@ -653,8 +691,14 @@ * * @see #getSeed */ - public synchronized void setSeed(byte[] seed) { - secureRandomSpi.engineSetSeed(seed); + public void setSeed(byte[] seed) { + if (threadSafe) { + secureRandomSpi.engineSetSeed(seed); + } else { + synchronized (this) { + secureRandomSpi.engineSetSeed(seed); + } + } } /** @@ -679,7 +723,7 @@ * yet been initialized at that point. */ if (seed != 0) { - this.secureRandomSpi.engineSetSeed(longToByteArray(seed)); + setSeed(longToByteArray(seed)); } } @@ -690,7 +734,13 @@ */ @Override public void nextBytes(byte[] bytes) { - secureRandomSpi.engineNextBytes(bytes); + if (threadSafe) { + secureRandomSpi.engineNextBytes(bytes); + } else { + synchronized (this) { + secureRandomSpi.engineNextBytes(bytes); + } + } } /** @@ -707,12 +757,19 @@ * * @since 9 */ - public synchronized void nextBytes( - byte[] bytes, SecureRandomParameters params) { + public void nextBytes(byte[] bytes, SecureRandomParameters params) { if (params == null) { throw new IllegalArgumentException("params cannot be null"); } - secureRandomSpi.engineNextBytes(Objects.requireNonNull(bytes), params); + if (threadSafe) { + secureRandomSpi.engineNextBytes( + Objects.requireNonNull(bytes), params); + } else { + synchronized (this) { + secureRandomSpi.engineNextBytes( + Objects.requireNonNull(bytes), params); + } + } } /** @@ -756,6 +813,7 @@ * * @param numBytes the number of seed bytes to generate. * + * @throws IllegalArgumentException if {@code numBytes} is negative * @return the seed bytes. * * @see #setSeed @@ -782,7 +840,13 @@ if (numBytes < 0) { throw new IllegalArgumentException("numBytes cannot be negative"); } - return secureRandomSpi.engineGenerateSeed(numBytes); + if (threadSafe) { + return secureRandomSpi.engineGenerateSeed(numBytes); + } else { + synchronized (this) { + return secureRandomSpi.engineGenerateSeed(numBytes); + } + } } /** @@ -917,8 +981,14 @@ * * @since 9 */ - public synchronized void reseed() { - secureRandomSpi.engineReseed(null); + public void reseed() { + if (threadSafe) { + secureRandomSpi.engineReseed(null); + } else { + synchronized (this) { + secureRandomSpi.engineReseed(null); + } + } } /** @@ -937,11 +1007,17 @@ * * @since 9 */ - public synchronized void reseed(SecureRandomParameters params) { + public void reseed(SecureRandomParameters params) { if (params == null) { throw new IllegalArgumentException("params cannot be null"); } - secureRandomSpi.engineReseed(params); + if (threadSafe) { + secureRandomSpi.engineReseed(params); + } else { + synchronized (this) { + secureRandomSpi.engineReseed(params); + } + } } // Declare serialVersionUID to be compatible with JDK1.1 diff -r c229da92b1a9 -r 5b0b84715c06 jdk/src/java.base/share/classes/java/security/SecureRandomSpi.java --- a/jdk/src/java.base/share/classes/java/security/SecureRandomSpi.java Thu Nov 17 11:55:59 2016 +0000 +++ b/jdk/src/java.base/share/classes/java/security/SecureRandomSpi.java Thu Nov 17 21:02:39 2016 +0800 @@ -58,6 +58,26 @@ * a {@code SecureRandomParameters} argument will never * return an instance of this implementation. The * {@link #engineGetParameters()} method must return {@code null}. + *

+ * See {@link SecureRandom} for additional details on thread safety. By + * default, a {@code SecureRandomSpi} implementation is considered to be + * not safe for use by multiple concurrent threads and {@code SecureRandom} + * will synchronize access to each of the applicable engine methods + * (see {@link SecureRandom} for the list of methods). However, if a + * {@code SecureRandomSpi} implementation is thread-safe, the + * service provider attribute "ThreadSafe" should be set to "true" during + * its registration, as follows: + * 

+ * put("SecureRandom.AlgName ThreadSafe", "true");
+ *
+ * or + * 
+ * putService(new Service(this, "SecureRandom", "AlgName", className,
+ *          null, Map.of("ThreadSafe", "true")));
+ *
+ * {@code SecureRandom} will call the applicable engine methods + * without any synchronization. * * @since 1.2 */ diff -r c229da92b1a9 -r 5b0b84715c06 jdk/src/java.base/share/classes/sun/security/provider/SunEntries.java --- a/jdk/src/java.base/share/classes/sun/security/provider/SunEntries.java Thu Nov 17 11:55:59 2016 +0000 +++ b/jdk/src/java.base/share/classes/sun/security/provider/SunEntries.java Thu Nov 17 21:02:39 2016 +0800 @@ -96,25 +96,32 @@ if (nativeAvailable && useNativePRNG) { map.put("SecureRandom.NativePRNG", "sun.security.provider.NativePRNG"); + map.put("SecureRandom.NativePRNG ThreadSafe", "true"); } map.put("SecureRandom.DRBG", "sun.security.provider.DRBG"); + map.put("SecureRandom.DRBG ThreadSafe", "true"); map.put("SecureRandom.SHA1PRNG", "sun.security.provider.SecureRandom"); + + map.put("SecureRandom.SHA1PRNG ThreadSafe", "true"); if (nativeAvailable && !useNativePRNG) { map.put("SecureRandom.NativePRNG", "sun.security.provider.NativePRNG"); + map.put("SecureRandom.NativePRNG ThreadSafe", "true"); } if (NativePRNG.Blocking.isAvailable()) { map.put("SecureRandom.NativePRNGBlocking", "sun.security.provider.NativePRNG$Blocking"); + map.put("SecureRandom.NativePRNGBlocking ThreadSafe", "true"); } if (NativePRNG.NonBlocking.isAvailable()) { map.put("SecureRandom.NativePRNGNonBlocking", "sun.security.provider.NativePRNG$NonBlocking"); + map.put("SecureRandom.NativePRNGNonBlocking ThreadSafe", "true"); } /* @@ -329,6 +336,7 @@ map.put("AlgorithmParameters.DSA ImplementedIn", "Software"); map.put("KeyFactory.DSA ImplementedIn", "Software"); map.put("SecureRandom.SHA1PRNG ImplementedIn", "Software"); + map.put("SecureRandom.DRBG ImplementedIn", "Software"); map.put("CertificateFactory.X.509 ImplementedIn", "Software"); map.put("KeyStore.JKS ImplementedIn", "Software"); map.put("CertPathValidator.PKIX ImplementedIn", "Software"); diff -r c229da92b1a9 -r 5b0b84715c06 jdk/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/SunMSCAPI.java --- a/jdk/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/SunMSCAPI.java Thu Nov 17 11:55:59 2016 +0000 +++ b/jdk/src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/SunMSCAPI.java Thu Nov 17 21:02:39 2016 +0800 @@ -33,6 +33,8 @@ import java.security.ProviderException; import java.util.HashMap; import java.util.Arrays; +import java.util.Map; + import static sun.security.util.SecurityConstants.PROVIDER_VER; /** @@ -133,8 +135,11 @@ /* * Secure random */ + HashMap srattrs = new HashMap<>(1); + srattrs.put("ThreadSafe", "true"); putService(new ProviderService(p, "SecureRandom", - "Windows-PRNG", "sun.security.mscapi.PRNG")); + "Windows-PRNG", "sun.security.mscapi.PRNG", + null, srattrs)); /* * Key store diff -r c229da92b1a9 -r 5b0b84715c06 jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java --- a/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java Thu Nov 17 11:55:59 2016 +0000 +++ b/jdk/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java Thu Nov 17 21:02:39 2016 +0800 @@ -987,7 +987,8 @@ P11Service(Token token, String type, String algorithm, String className, String[] al, long mechanism) { - super(token.provider, type, algorithm, className, toList(al), null); + super(token.provider, type, algorithm, className, toList(al), + type.equals(SR) ? Map.of("ThreadSafe", "true") : null); this.token = token; this.mechanism = mechanism & 0xFFFFFFFFL; } diff -r c229da92b1a9 -r 5b0b84715c06 jdk/test/java/security/SecureRandom/NoSync.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/jdk/test/java/security/SecureRandom/NoSync.java Thu Nov 17 21:02:39 2016 +0800 @@ -0,0 +1,101 @@ +/* + * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +import java.security.Provider; +import java.security.SecureRandom; +import java.security.Security; +import java.util.Date; +import java.util.concurrent.atomic.AtomicBoolean; + +/* + * @test + * @bug 7004967 + * @run main/othervm NoSync + * @summary SecureRandom should be more explicit about threading + */ +public class NoSync { + public static void main(String[] args) throws Exception { + for (Provider p : Security.getProviders()) { + for (Provider.Service s : p.getServices()) { + if (s.getType().equals("SecureRandom") && + !s.getAlgorithm().contains("Block")) { + test(SecureRandom.getInstance(s.getAlgorithm(), p)); + } + } + } + Security.setProperty("securerandom.drbg.config", "HMAC_DRBG"); + test(SecureRandom.getInstance("DRBG")); + Security.setProperty("securerandom.drbg.config", "CTR_DRBG"); + test(SecureRandom.getInstance("DRBG")); + } + + static void test(SecureRandom sr) throws Exception { + test(sr, 20, 3000); + // All out-of-box impl should have the ThreadSafe attribute + String attr = sr.getProvider().getProperty("SecureRandom." + + sr.getAlgorithm() + " ThreadSafe"); + if (!"true".equals(attr)) { + throw new Exception("Not ThreadSafe: " + attr); + } + } + + public static void test(SecureRandom sr, int tnum, int rnum) + throws Exception { + + System.out.println(sr); + System.out.println(sr.getAlgorithm() + " " + sr.getProvider().getName()); + + System.out.println(new Date()); + boolean reseed = sr.getParameters() != null; + Thread[] threads = new Thread[tnum]; + AtomicBoolean failed = new AtomicBoolean(false); + Thread.UncaughtExceptionHandler h = (t, e) -> { + failed.set(true); + e.printStackTrace(); + }; + for (int i = 0; i < threads.length; i++) { + threads[i] = new Thread() { + @Override + public void run() { + for (int j = 0; j < rnum; j++) { + sr.nextBytes(new byte[j%100+100]); + sr.setSeed((long)j); + if (reseed) { + sr.reseed(); + } + } + } + }; + threads[i].setUncaughtExceptionHandler(h); + threads[i].start(); + } + for (int i = 0; i < threads.length; i++) { + threads[i].join(); + } + System.out.println(new Date()); + System.out.println(); + if (failed.get()) { + throw new RuntimeException("Failed"); + } + } +} diff -r c229da92b1a9 -r 5b0b84715c06 jdk/test/java/security/SecureRandom/ThreadSafe.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/jdk/test/java/security/SecureRandom/ThreadSafe.java Thu Nov 17 21:02:39 2016 +0800 @@ -0,0 +1,102 @@ +/* + * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +import java.security.Provider; +import java.security.SecureRandom; +import java.security.SecureRandomSpi; +import java.util.Map; + +/* + * @test + * @bug 7004967 + * @summary SecureRandom should be more explicit about threading + */ +public class ThreadSafe { + public static void main(String[] args) throws Exception { + Provider p = new P(); + NoSync.test(SecureRandom.getInstance("S1", p), 5, 5); + try { + NoSync.test(SecureRandom.getInstance("S2", p), 5, 5); + throw new Exception("Failed"); + } catch (RuntimeException re) { + // Good + } + NoSync.test(SecureRandom.getInstance("S3", p), 5, 5); + try { + NoSync.test(SecureRandom.getInstance("S4", p), 5, 5); + throw new Exception("Failed"); + } catch (RuntimeException re) { + // Good + } + } + + public static class P extends Provider { + public P() { + + super("P", 1.0d, "Haha"); + + // Good. No attribute. + put("SecureRandom.S1", S.class.getName()); + + // Bad. Boasting ThreadSafe but isn't + put("SecureRandom.S2", S.class.getName()); + put("SecureRandom.S2 ThreadSafe", "true"); + + // Good. No attribute. + putService(new Service(this, "SecureRandom", "S3", + S.class.getName(), null, null)); + + // Bad. Boasting ThreadSafe but isn't + putService(new Service(this, "SecureRandom", "S4", + S.class.getName(), null, Map.of("ThreadSafe", "true"))); + } + } + + // This implementation is not itself thread safe. + public static class S extends SecureRandomSpi { + @java.lang.Override + protected void engineSetSeed(byte[] seed) { + return; + } + + private volatile boolean inCall = false; + @Override + protected void engineNextBytes(byte[] bytes) { + if (inCall) { + throw new RuntimeException("IN CALL"); + } + inCall = true; + try { + Thread.sleep(100); + } catch (Exception e) { + // OK + } + inCall = false; + } + + @Override + protected byte[] engineGenerateSeed(int numBytes) { + return new byte[numBytes]; + } + } +}