On Dec 13, 2016, at 4:05 AM, Adam Petcher <adam.petc...@oracle.com>
wrote:
Okay. I changed getProvider() to return this.provider.orElse(null),
which will allow this method to return null. For consistency, I allow
all other providers (in Instance and Service) to be null using
Optional.ofNullable(). Hopefully, I found and fixed all the
whitespace issues, too. Here is the corrected diff:
diff --git a/src/java.base/share/classes/java/security/Signature.java
b/src/java.base/share/classes/java/security/Signature.java
--- a/src/java.base/share/classes/java/security/Signature.java
+++ b/src/java.base/share/classes/java/security/Signature.java
@@ -134,7 +134,7 @@
private String algorithm;
// The provider
- Provider provider;
+ Optional<Provider> provider = Optional.empty();
/**
* Possible {@link #state} value, signifying that
@@ -266,7 +266,7 @@
SignatureSpi spi = (SignatureSpi)instance.impl;
sig = new Delegate(spi, algorithm);
}
- sig.provider = instance.provider;
+ sig.provider = Optional.ofNullable(instance.provider);
return sig;
}
@@ -449,13 +449,17 @@
*/
public final Provider getProvider() {
chooseFirstProvider();
- return this.provider;
+ return this.provider.orElse(null);
}
void chooseFirstProvider() {
// empty, overridden in Delegate
}
+ private String getProvidedByString() {
+ return provider.map(x -> " from: " + x.getName()).orElse("");
+ }
+
/**
* Initializes this object for verification. If this method is
called
* again with a different argument, it negates the effect
@@ -473,7 +477,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " verification algorithm from: " +
this.provider.getName());
+ " verification algorithm" + getProvidedByString());
}
}
@@ -522,7 +526,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " verification algorithm from: " +
this.provider.getName());
+ " verification algorithm" + getProvidedByString());
}
}
@@ -543,7 +547,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " signing algorithm from: " + this.provider.getName());
+ " signing algorithm" + getProvidedByString());
}
}
@@ -566,7 +570,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " signing algorithm from: " + this.provider.getName());
+ " signing algorithm" + getProvidedByString());
}
}
@@ -1087,7 +1091,7 @@
}
try {
sigSpi = newInstance(s);
- provider = s.getProvider();
+ provider =
Optional.ofNullable(s.getProvider());
// not needed any more
firstService = null;
serviceIterator = null;
@@ -1132,7 +1136,7 @@
try {
SignatureSpi spi = newInstance(s);
init(spi, type, key, random);
- provider = s.getProvider();
+ provider =
Optional.ofNullable(s.getProvider());
sigSpi = spi;
firstService = null;
serviceIterator = null;
diff --git a/test/java/security/Signature/NoProvider.java
b/test/java/security/Signature/NoProvider.java
new file mode 100644
--- /dev/null
+++ b/test/java/security/Signature/NoProvider.java
@@ -0,0 +1,99 @@
+/*
+ * 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.
+ */
+
+/*
+ * @test
+ * @bug 8165751
+ * @summary Verify that that a subclass of Signature that does not
contain a
+ * provider can be used verify.
+ * @run main/othervm -Djava.security.debug=provider NoProvider
+ */
+
+import java.security.*;
+
+public class NoProvider {
+
+ private static class NoProviderPublicKey implements PublicKey {
+
+ public String getAlgorithm() {
+ return "NoProvider";
+ }
+ public String getFormat() {
+ return "none";
+ }
+ public byte[] getEncoded() {
+ return new byte[1];
+ }
+ }
+
+ private static class NoProviderSignature extends Signature {
+
+ public NoProviderSignature() {
+ super("NoProvider");
+ }
+
+ protected void engineInitVerify(PublicKey publicKey)
+ throws InvalidKeyException {
+ // do nothing
+ }
+
+ protected void engineInitSign(PrivateKey privateKey)
+ throws InvalidKeyException {
+ // do nothing
+ }
+
+ protected void engineUpdate(byte b) throws SignatureException {
+ // do nothing
+ }
+
+ protected void engineUpdate(byte[] b, int off, int len)
+ throws SignatureException {
+ // do nothing
+ }
+
+ protected byte[] engineSign() throws SignatureException {
+ return new byte[1];
+ }
+
+ protected boolean engineVerify(byte[] sigBytes)
+ throws SignatureException {
+ return false;
+ }
+
+ @Deprecated
+ protected void engineSetParameter(String param, Object value)
+ throws InvalidParameterException {
+ // do nothing
+ }
+
+ @Deprecated
+ protected Object engineGetParameter(String param)
+ throws InvalidParameterException {
+ return null;
+ }
+ }
+
+ public static void main(String[] args) throws Exception {
+ new NoProviderSignature().initVerify(new
NoProviderPublicKey());
+ }
+}
On 12/12/2016 1:44 PM, Sean Mullan wrote:
On 12/8/16 11:17 AM, Adam Petcher wrote:
Subclasses of Signature may have a null provider. In this case, the
debug logging code will throw a NPE when attempting to call
getName() on
it. Since this member may be null, I would like to change its type to
Optional to ensure that code checks whether it is present before using
it. I have assumed that getProvider() methods (in both Signature and
Service) always return non-null---if this assumption is incorrect,
then
I'll need to change some of the uses of Optional in/around these
methods.
I think we would want to preserve the current behavior of
getProvider returning null for one of these subclasses (even though
it isn't specified that null is an acceptable return value). Better
to be safe here, otherwise NoSuchElementException would be thrown
which would be unexpected and not specified by getProvider.
Other than that, just a minor style comment on a few lines of the test:
+ private static class NoProviderPublicKey implements PublicKey{
space before "{"
--Sean
Note that this issue of missing null checks for providers exists in
several classes (e.g. Cipher, MessageDigest). Once this patch is
reviewed and committed, I will apply the same solution to all other
affected classes.
Bug: https://bugs.openjdk.java.net/browse/JDK-8165751
Diff:
diff --git a/src/java.base/share/classes/java/security/Signature.java
b/src/java.base/share/classes/java/security/Signature.java
--- a/src/java.base/share/classes/java/security/Signature.java
+++ b/src/java.base/share/classes/java/security/Signature.java
@@ -134,7 +134,7 @@
private String algorithm;
// The provider
- Provider provider;
+ Optional<Provider> provider = Optional.empty();
/**
* Possible {@link #state} value, signifying that
@@ -266,7 +266,7 @@
SignatureSpi spi = (SignatureSpi)instance.impl;
sig = new Delegate(spi, algorithm);
}
- sig.provider = instance.provider;
+ sig.provider = Optional.of(instance.provider);
return sig;
}
@@ -449,13 +449,17 @@
*/
public final Provider getProvider() {
chooseFirstProvider();
- return this.provider;
+ return this.provider.get();
}
void chooseFirstProvider() {
// empty, overridden in Delegate
}
+ private String getProvidedByString(){
+ return provider.map(x -> " from: " + x.getName()).orElse("");
+ }
+
/**
* Initializes this object for verification. If this method is
called
* again with a different argument, it negates the effect
@@ -473,7 +477,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " verification algorithm from: " +
this.provider.getName());
+ " verification algorithm" + getProvidedByString());
}
}
@@ -522,7 +526,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " verification algorithm from: " +
this.provider.getName());
+ " verification algorithm" + getProvidedByString());
}
}
@@ -543,7 +547,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " signing algorithm from: " +
this.provider.getName());
+ " signing algorithm" + getProvidedByString());
}
}
@@ -566,7 +570,7 @@
if (!skipDebug && pdebug != null) {
pdebug.println("Signature." + algorithm +
- " signing algorithm from: " +
this.provider.getName());
+ " signing algorithm" + getProvidedByString());
}
}
@@ -1087,7 +1091,7 @@
}
try {
sigSpi = newInstance(s);
- provider = s.getProvider();
+ provider = Optional.of(s.getProvider());
// not needed any more
firstService = null;
serviceIterator = null;
@@ -1132,7 +1136,7 @@
try {
SignatureSpi spi = newInstance(s);
init(spi, type, key, random);
- provider = s.getProvider();
+ provider = Optional.of(s.getProvider());
sigSpi = spi;
firstService = null;
serviceIterator = null;
diff --git a/test/java/security/Signature/NoProvider.java
b/test/java/security/Signature/NoProvider.java
new file mode 100644
--- /dev/null
+++ b/test/java/security/Signature/NoProvider.java
@@ -0,0 +1,99 @@
+/*
+ * 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.
+ */
+
+/*
+ * @test
+ * @bug 8165751
+ * @summary Verify that that a subclass of Signature that does not
contain a
+ * provider can be used verify.
+ * @run main/othervm -Djava.security.debug=provider NoProvider
+ */
+
+import java.security.*;
+
+public class NoProvider {
+
+ private static class NoProviderPublicKey implements PublicKey{
+ public String getAlgorithm(){
+ return "NoProvider";
+ }
+ public String getFormat(){
+ return "none";
+ }
+ public byte[] getEncoded(){
+ return new byte[1];
+ }
+ }
+
+ private static class NoProviderSignature extends Signature{
+
+ public NoProviderSignature(){
+ super("NoProvider");
+ }
+
+ protected void engineInitVerify(PublicKey publicKey)
+ throws InvalidKeyException{
+ // do nothing
+ }
+
+ protected void engineInitSign(PrivateKey privateKey)
+ throws InvalidKeyException{
+ // do nothing
+ }
+
+ protected void engineUpdate(byte b) throws
SignatureException{
+ // do nothing
+ }
+
+ protected void engineUpdate(byte[] b, int off, int len)
+ throws SignatureException{
+ // do nothing
+ }
+
+ protected byte[] engineSign() throws SignatureException{
+ return new byte[1];
+ }
+
+ protected boolean engineVerify(byte[] sigBytes)
+ throws SignatureException{
+ return false;
+ }
+
+ @Deprecated
+ protected void engineSetParameter(String param, Object value)
+ throws InvalidParameterException{
+ // do nothing
+ }
+
+ @Deprecated
+ protected Object engineGetParameter(String param)
+ throws InvalidParameterException{
+ return null;
+ }
+
+ }
+
+ public static void main(String[] args) throws Exception {
+ new NoProviderSignature().initVerify(new
NoProviderPublicKey());
+ }
+}
