[PATCH] 8165751: NPE in Signature with java.security.debug=provider
Adam Petcher
adam.petcher at oracle.com
Mon Dec 12 20:05:02 UTC 2016
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());
>> + }
>> +}
More information about the security-dev
mailing list