HBASE-26582 Prune use of Random and SecureRandom objects (#4118)

Avoid the pattern where a Random object is allocated, used once or twice, and
then left for GC. This pattern triggers warnings from some static analysis tools
because this pattern leads to poor effective randomness. In a few cases we were
legitimately suffering from this issue; in others a change is still good to
reduce noise in analysis results.

Use ThreadLocalRandom where there is no requirement to set the seed to gain
good reuse.

Where useful relax use of SecureRandom to simply Random or ThreadLocalRandom,
which are unlikely to block if the system entropy pool is low, if we don't need
crypographically strong randomness for the use case. The exception to this is
normalization of use of Bytes#random to fill byte arrays with randomness.
Because Bytes#random may be used to generate key material it must be backed by
SecureRandom.

Signed-off-by: Duo Zhang <[email protected]>

Author: apurtell

hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/io/asyncfs/TestFanOutOneBlockAsyncDFSOutput.java

@@ -34,7 +34,6 @@
 import java.util.Random;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ThreadLocalRandom;
 import org.apache.hadoop.fs.FSDataInputStream;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
@@ -44,6 +43,7 @@
 import org.apache.hadoop.hbase.io.asyncfs.monitor.StreamSlowMonitor;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hdfs.DistributedFileSystem;
 import org.apache.hadoop.hdfs.MiniDFSCluster.DataNodeProperties;
 import org.apache.hadoop.hdfs.server.datanode.DataNode;
@@ -57,7 +57,6 @@
 import org.junit.rules.TestName;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.io.netty.channel.Channel;
 import org.apache.hbase.thirdparty.io.netty.channel.EventLoop;
 import org.apache.hbase.thirdparty.io.netty.channel.EventLoopGroup;
@@ -72,13 +71,9 @@ public class TestFanOutOneBlockAsyncDFSOutput extends AsyncFSTestBase {
     HBaseClassTestRule.forClass(TestFanOutOneBlockAsyncDFSOutput.class);
 
   private static final Logger LOG = LoggerFactory.getLogger(TestFanOutOneBlockAsyncDFSOutput.class);
-
   private static DistributedFileSystem FS;
-
   private static EventLoopGroup EVENT_LOOP_GROUP;
-
   private static Class<? extends Channel> CHANNEL_CLASS;
-
   private static int READ_TIMEOUT_MS = 2000;
 
   private static StreamSlowMonitor MONITOR;
@@ -104,14 +99,16 @@ public static void tearDown() throws IOException, InterruptedException {
     shutdownMiniDFSCluster();
   }
 
+  private static final Random RNG = new Random(); // This test depends on Random#setSeed
+
   static void writeAndVerify(FileSystem fs, Path f, AsyncFSOutput out)
     throws IOException, InterruptedException, ExecutionException {
     List<CompletableFuture<Long>> futures = new ArrayList<>();
     byte[] b = new byte[10];
-    Random rand = new Random(12345);
     // test pipelined flush
+    RNG.setSeed(12345);
     for (int i = 0; i < 10; i++) {
-      rand.nextBytes(b);
+      RNG.nextBytes(b);
       out.write(b);
       futures.add(out.flush(false));
       futures.add(out.flush(false));
@@ -123,11 +120,11 @@ static void writeAndVerify(FileSystem fs, Path f, AsyncFSOutput out)
     out.close();
     assertEquals(b.length * 10, fs.getFileStatus(f).getLen());
     byte[] actual = new byte[b.length];
-    rand.setSeed(12345);
+    RNG.setSeed(12345);
     try (FSDataInputStream in = fs.open(f)) {
       for (int i = 0; i < 10; i++) {
         in.readFully(actual);
-        rand.nextBytes(b);
+        RNG.nextBytes(b);
         assertArrayEquals(b, actual);
       }
       assertEquals(-1, in.read());
@@ -150,7 +147,7 @@ public void testRecover() throws IOException, InterruptedException, ExecutionExc
     FanOutOneBlockAsyncDFSOutput out = FanOutOneBlockAsyncDFSOutputHelper.createOutput(FS, f, true,
       false, (short) 3, FS.getDefaultBlockSize(), eventLoop, CHANNEL_CLASS, MONITOR);
     byte[] b = new byte[10];
-    ThreadLocalRandom.current().nextBytes(b);
+    Bytes.random(b);
     out.write(b, 0, b.length);
     out.flush(false).get();
     // restart one datanode which causes one connection broken
@@ -260,7 +257,7 @@ public void testWriteLargeChunk() throws IOException, InterruptedException, Exec
     FanOutOneBlockAsyncDFSOutput out = FanOutOneBlockAsyncDFSOutputHelper.createOutput(FS, f, true,
       false, (short) 3, 1024 * 1024 * 1024, eventLoop, CHANNEL_CLASS, MONITOR);
     byte[] b = new byte[50 * 1024 * 1024];
-    ThreadLocalRandom.current().nextBytes(b);
+    Bytes.random(b);
     out.write(b);
     out.flush(false);
     assertEquals(b.length, out.flush(false).get().longValue());

hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/io/asyncfs/TestFanOutOneBlockAsyncDFSOutputHang.java

@@ -17,7 +17,6 @@
  */
 package org.apache.hadoop.hbase.io.asyncfs;
 
-
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -29,12 +28,12 @@
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ThreadLocalRandom;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.io.asyncfs.monitor.StreamSlowMonitor;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hdfs.DistributedFileSystem;
 import org.apache.hadoop.hdfs.MiniDFSCluster.DataNodeProperties;
 import org.apache.hadoop.hdfs.protocol.DatanodeInfo;
@@ -57,7 +56,6 @@
 import org.apache.hbase.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
 import org.apache.hbase.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
 
-
 /**
  * Testcase for HBASE-26679, here we introduce a separate test class and not put the testcase in
  * {@link TestFanOutOneBlockAsyncDFSOutput} because we will send heartbeat to DN when there is no
@@ -191,7 +189,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
       });
 
       byte[] b = new byte[10];
-      ThreadLocalRandom.current().nextBytes(b);
+      Bytes.random(b);
       OUT.write(b, 0, b.length);
       CompletableFuture<Long> future = OUT.flush(false);
       /**

hbase-client/src/main/java/org/apache/hadoop/hbase/client/PerClientRandomNonceGenerator.java

@@ -19,8 +19,7 @@
 package org.apache.hadoop.hbase.client;
 
 import java.util.Arrays;
-import java.util.Random;
-
+import java.util.concurrent.ThreadLocalRandom;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.yetus.audience.InterfaceAudience;
 
@@ -33,12 +32,12 @@ public final class PerClientRandomNonceGenerator implements NonceGenerator {
 
   private static final PerClientRandomNonceGenerator INST = new PerClientRandomNonceGenerator();
 
-  private final Random rdm = new Random();
   private final long clientId;
 
   private PerClientRandomNonceGenerator() {
     byte[] clientIdBase = ClientIdGenerator.generateClientId();
-    this.clientId = (((long) Arrays.hashCode(clientIdBase)) << 32) + rdm.nextInt();
+    this.clientId = (((long) Arrays.hashCode(clientIdBase)) << 32) +
+      ThreadLocalRandom.current().nextInt();
   }
 
   @Override
@@ -50,7 +49,7 @@ public long getNonceGroup() {
   public long newNonce() {
     long result = HConstants.NO_NONCE;
     do {
-      result = rdm.nextLong();
+      result = ThreadLocalRandom.current().nextLong();
     } while (result == HConstants.NO_NONCE);
     return result;
   }

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RandomRowFilter.java

@@ -20,7 +20,7 @@
 package org.apache.hadoop.hbase.filter;
 
 import java.util.Objects;
-import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
 
 import org.apache.hadoop.hbase.Cell;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -35,7 +35,6 @@
  */
 @InterfaceAudience.Public
 public class RandomRowFilter extends FilterBase {
-  protected static final Random random = new Random();
 
   protected float chance;
   protected boolean filterOutRow;
@@ -104,7 +103,7 @@ public boolean filterRowKey(Cell firstRowCell) {
       filterOutRow = false;
     } else {
       // roll the dice
-      filterOutRow = !(random.nextFloat() < chance);
+      filterOutRow = !(ThreadLocalRandom.current().nextFloat() < chance);
     }
     return filterOutRow;
   }

hbase-client/src/main/java/org/apache/hadoop/hbase/security/EncryptionUtil.java

@@ -22,7 +22,6 @@
 import java.io.IOException;
 import java.security.Key;
 import java.security.KeyException;
-import java.security.SecureRandom;
 import java.util.Properties;
 import javax.crypto.spec.SecretKeySpec;
 import org.apache.commons.crypto.cipher.CryptoCipherFactory;
@@ -37,7 +36,6 @@
 import org.apache.yetus.audience.InterfaceStability;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.protobuf.UnsafeByteOperations;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.EncryptionProtos;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos;
@@ -50,8 +48,6 @@
 public final class EncryptionUtil {
   static private final Logger LOG = LoggerFactory.getLogger(EncryptionUtil.class);
 
-  static private final SecureRandom RNG = new SecureRandom();
-
   /**
    * Private constructor to keep this class from being instantiated.
    */
@@ -96,7 +92,7 @@ public static byte[] wrapKey(Configuration conf, String subject, Key key)
     byte[] iv = null;
     if (cipher.getIvLength() > 0) {
       iv = new byte[cipher.getIvLength()];
-      RNG.nextBytes(iv);
+      Bytes.secureRandom(iv);
       builder.setIv(UnsafeByteOperations.unsafeWrap(iv));
     }
     byte[] keyBytes = key.getEncoded();
@@ -286,7 +282,7 @@ public static Key unwrapKey(Configuration conf, byte[] keyBytes) throws IOExcept
    * @throws IOException if create CryptoAES failed
    */
   public static CryptoAES createCryptoAES(RPCProtos.CryptoCipherMeta cryptoCipherMeta,
-                               Configuration conf) throws IOException {
+      Configuration conf) throws IOException {
     Properties properties = new Properties();
     // the property for cipher class
     properties.setProperty(CryptoCipherFactory.CLASSES_KEY,

hbase-client/src/main/java/org/apache/hadoop/hbase/slowlog/SlowLogTableAccessor.java

@@ -22,7 +22,8 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Random;
+import java.util.concurrent.ThreadLocalRandom;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.NamespaceDescriptor;
@@ -49,8 +50,6 @@ public class SlowLogTableAccessor {
 
   private static final Logger LOG = LoggerFactory.getLogger(SlowLogTableAccessor.class);
 
-  private static final Random RANDOM = new Random();
-
   private static Connection connection;
 
   /**
@@ -139,7 +138,7 @@ private static byte[] getRowKey(final TooSlowLog.SlowLogPayload slowLogPayload)
     String lastFiveDig =
       hashcode.substring((hashcode.length() > 5) ? (hashcode.length() - 5) : 0);
     if (lastFiveDig.startsWith("-")) {
-      lastFiveDig = String.valueOf(RANDOM.nextInt(99999));
+      lastFiveDig = String.valueOf(ThreadLocalRandom.current().nextInt(99999));
     }
     final long currentTime = EnvironmentEdgeManager.currentTime();
     final String timeAndHashcode = currentTime + lastFiveDig;

hbase-client/src/test/java/org/apache/hadoop/hbase/security/TestEncryptionUtil.java

@@ -23,7 +23,7 @@
 
 import java.security.Key;
 import java.security.KeyException;
-import java.security.SecureRandom;
+
 import javax.crypto.spec.SecretKeySpec;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -110,7 +110,7 @@ public void testWALKeyWrappingWithIncorrectKey() throws Exception {
 
     // generate a test key
     byte[] keyBytes = new byte[AES.KEY_LENGTH];
-    new SecureRandom().nextBytes(keyBytes);
+    Bytes.secureRandom(keyBytes);
     String algorithm = conf.get(HConstants.CRYPTO_WAL_ALGORITHM_CONF_KEY, HConstants.CIPHER_AES);
     Key key = new SecretKeySpec(keyBytes, algorithm);
 
@@ -152,7 +152,7 @@ private void testKeyWrapping(String hashAlgorithm) throws Exception {
 
     // generate a test key
     byte[] keyBytes = new byte[AES.KEY_LENGTH];
-    new SecureRandom().nextBytes(keyBytes);
+    Bytes.secureRandom(keyBytes);
     String algorithm =
       conf.get(HConstants.CRYPTO_KEY_ALGORITHM_CONF_KEY, HConstants.CIPHER_AES);
     Key key = new SecretKeySpec(keyBytes, algorithm);
@@ -189,7 +189,7 @@ private void testWALKeyWrapping(String hashAlgorithm) throws Exception {
 
     // generate a test key
     byte[] keyBytes = new byte[AES.KEY_LENGTH];
-    new SecureRandom().nextBytes(keyBytes);
+    Bytes.secureRandom(keyBytes);
     String algorithm = conf.get(HConstants.CRYPTO_WAL_ALGORITHM_CONF_KEY, HConstants.CIPHER_AES);
     Key key = new SecretKeySpec(keyBytes, algorithm);
 
@@ -214,7 +214,7 @@ private void testKeyWrappingWithMismatchingAlgorithms(Configuration conf) throws
 
     // generate a test key
     byte[] keyBytes = new byte[AES.KEY_LENGTH];
-    new SecureRandom().nextBytes(keyBytes);
+    Bytes.secureRandom(keyBytes);
     String algorithm =
       conf.get(HConstants.CRYPTO_KEY_ALGORITHM_CONF_KEY, HConstants.CIPHER_AES);
     Key key = new SecretKeySpec(keyBytes, algorithm);

hbase-client/src/test/java/org/apache/hadoop/hbase/util/TestRoundRobinPoolMap.java

@@ -25,11 +25,9 @@
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Random;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.MiscTests;

hbase-client/src/test/java/org/apache/hadoop/hbase/util/TestThreadLocalPoolMap.java

@@ -20,11 +20,9 @@
 import static org.junit.Assert.assertEquals;
 
 import java.io.IOException;
-import java.util.Random;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.testclassification.MiscTests;

hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Encryption.java

@@ -317,7 +317,7 @@ public static byte[] generateSecretKey(Configuration conf, String cypherAlg, byt
    */
   private static byte[] generateSecretKey(String algorithm, int keyLengthBytes, char[] password) {
     byte[] salt = new byte[keyLengthBytes];
-    Bytes.random(salt);
+    Bytes.secureRandom(salt);
     PBEKeySpec spec = new PBEKeySpec(password, salt, 10000, keyLengthBytes*8);
     try {
       return SecretKeyFactory.getInstance(algorithm).generateSecret(spec).getEncoded();