Re: [PATCH] 8170876: NPE in Cipher with java.security.debug=provider

[Adam Petcher]

KeyPairGenerator didn't require any changes because that class does not 
reference its provider member. KeyPairGenerator.Delegate has similar 
code that prints the name of its provider, but this is not an instance 
of the pattern that I was addressing, and I don't have any reason to 
believe there is anything wrong with it.

Though maybe I missed something here. If you still think this class 
needs a modification, please point me to a line number that has this issue.


On 12/20/2016 4:02 PM, Sean Mullan wrote:
I think you missed java.security.KeyPairGenerator which has the same 
issue. Otherwise looks good.

--Sean

On 12/15/16 3:08 PM, Adam Petcher wrote:
I'm continuing my quest to rid engine classes of NullPointerException
due to calling getName() on a null provider. This patch fixes Cipher
(which fails in a test using NullCipher) and all other engine classes
that have this pattern. I found them by searching the codebase for
references to Provider::getName. This fix does not address NPEs caused
by calls to other methods of Provider, or calls that occur outside the
engine classes (e.g. someEngine.getProvider().getName()).

I augmented an existing test so it will check for the NPE in Cipher. The
diff also containsa fix for a small typo in the NullProvider test for
Signature.

Bug: https://bugs.openjdk.java.net/browse/JDK-8170876

Diff:

diff --git a/src/java.base/share/classes/java/security/KeyStore.java
b/src/java.base/share/classes/java/security/KeyStore.java
--- a/src/java.base/share/classes/java/security/KeyStore.java
+++ b/src/java.base/share/classes/java/security/KeyStore.java
@@ -824,10 +824,14 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("KeyStore." + type.toUpperCase() + " type
from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Returns a keystore object of the specified type.
      *
diff --git
a/src/java.base/share/classes/java/security/MessageDigest.java
b/src/java.base/share/classes/java/security/MessageDigest.java
--- a/src/java.base/share/classes/java/security/MessageDigest.java
+++ b/src/java.base/share/classes/java/security/MessageDigest.java
@@ -430,13 +430,17 @@
         return digest();
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Returns a string representation of this message digest object.
      */
     public String toString() {
         ByteArrayOutputStream baos = new ByteArrayOutputStream();
         PrintStream p = new PrintStream(baos);
-        p.print(algorithm+" Message Digest from "+provider.getName()+",
");
+        p.print(algorithm+" Message Digest from 
"+getProviderName()+", ");
         switch (state) {
         case INITIAL:
             p.print("<initialized>");
diff --git a/src/java.base/share/classes/java/security/SecureRandom.java
b/src/java.base/share/classes/java/security/SecureRandom.java
--- a/src/java.base/share/classes/java/security/SecureRandom.java
+++ b/src/java.base/share/classes/java/security/SecureRandom.java
@@ -310,10 +310,14 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("SecureRandom." + algorithm +
-                " algorithm from: " + this.provider.getName());
+                " algorithm from: " + getProviderName());
         }
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Returns a {@code SecureRandom} object that implements the 
specified
      * Random Number Generator (RNG) algorithm.
diff --git a/src/java.base/share/classes/javax/crypto/Cipher.java
b/src/java.base/share/classes/javax/crypto/Cipher.java
--- a/src/java.base/share/classes/javax/crypto/Cipher.java
+++ b/src/java.base/share/classes/javax/crypto/Cipher.java
@@ -611,6 +611,10 @@
         return getInstance(transformation, p);
     }

+    private String getProviderName() {
+        return (provider == null)  ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Returns a {@code Cipher} object that implements the specified
      * transformation.
@@ -1278,7 +1282,7 @@
         if (!skipDebug && pdebug != null) {
             pdebug.println("Cipher." + transformation + " " +
                 getOpmodeString(opmode) + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -1421,7 +1425,7 @@
         if (!skipDebug && pdebug != null) {
             pdebug.println("Cipher." + transformation + " " +
                 getOpmodeString(opmode) + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -1564,7 +1568,7 @@
         if (!skipDebug && pdebug != null) {
             pdebug.println("Cipher." + transformation + " " +
                 getOpmodeString(opmode) + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -1754,7 +1758,7 @@
         if (!skipDebug && pdebug != null) {
             pdebug.println("Cipher." + transformation + " " +
                 getOpmodeString(opmode) + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

diff --git a/src/java.base/share/classes/javax/crypto/KeyAgreement.java
b/src/java.base/share/classes/javax/crypto/KeyAgreement.java
--- a/src/java.base/share/classes/javax/crypto/KeyAgreement.java
+++ b/src/java.base/share/classes/javax/crypto/KeyAgreement.java
@@ -484,7 +484,7 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("KeyAgreement." + algorithm + " algorithm
from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -517,6 +517,10 @@
         init(key, params, JceSecurity.RANDOM);
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Initializes this key agreement with the given key, set of
      * algorithm parameters, and source of randomness.
@@ -545,7 +549,7 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("KeyAgreement." + algorithm + " algorithm
from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

diff --git a/src/java.base/share/classes/javax/crypto/KeyGenerator.java
b/src/java.base/share/classes/javax/crypto/KeyGenerator.java
--- a/src/java.base/share/classes/javax/crypto/KeyGenerator.java
+++ b/src/java.base/share/classes/javax/crypto/KeyGenerator.java
@@ -154,7 +154,7 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("KeyGenerator." + algorithm + " algorithm
from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -172,10 +172,14 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("KeyGenerator." + algorithm + " algorithm
from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Returns the algorithm name of this {@code KeyGenerator} object.
      *
diff --git a/src/java.base/share/classes/javax/crypto/Mac.java
b/src/java.base/share/classes/javax/crypto/Mac.java
--- a/src/java.base/share/classes/javax/crypto/Mac.java
+++ b/src/java.base/share/classes/javax/crypto/Mac.java
@@ -415,6 +415,10 @@
         return spi.engineGetMacLength();
     }

+    private String getProviderName() {
+        return (provider == null) ? "(no provider)" : 
provider.getName();
+    }
+
     /**
      * Initializes this {@code Mac} object with the given key.
      *
@@ -437,7 +441,7 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("Mac." + algorithm + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

@@ -464,7 +468,7 @@

         if (!skipDebug && pdebug != null) {
             pdebug.println("Mac." + algorithm + " algorithm from: " +
-                this.provider.getName());
+                getProviderName());
         }
     }

diff --git a/test/java/security/Signature/NoProvider.java
b/test/java/security/Signature/NoProvider.java
--- a/test/java/security/Signature/NoProvider.java
+++ b/test/java/security/Signature/NoProvider.java
@@ -25,7 +25,7 @@
  * @test
  * @bug 8165751
  * @summary Verify that that a subclass of Signature that does not
contain a
- *     provider can be used verify.
+ *     provider can be used to verify.
  * @run main/othervm -Djava.security.debug=provider NoProvider
  */

diff --git a/test/javax/crypto/NullCipher/TestNPE.java
b/test/javax/crypto/NullCipher/TestNPE.java
--- a/test/javax/crypto/NullCipher/TestNPE.java
+++ b/test/javax/crypto/NullCipher/TestNPE.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2003, 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
@@ -23,10 +23,12 @@

 /*
  * @test
- * @bug 4937853
+ * @bug 4937853 8170876
  * @summary Make sure normal calls of NullCipher does not throw NPE.
  * @author Valerie Peng
  * @key randomness
+ * @run main TestNPE
+ * @run main/othervm -Djava.security.debug=provider TestNPE
  */
 import java.util.Arrays;
 import java.security.AlgorithmParameters;