[PATCH] 8165751: NPE in Signature with java.security.debug=provider
Wang Weijun
weijun.wang at oracle.com
Tue Dec 13 02:20:46 UTC 2016
Hi Adam
The only behavior change is with the debug output, right?
Is this a new pattern that internal optional fields should be defined as an Optional?
And, when there is no provider the string form "from: " looks strange, I would rather make it "from nowhere". I would also move the space before "from" to where the method is called, say, " verification algorithm " + getProvidedByString().
Thanks
Max
> On Dec 13, 2016, at 4:05 AM, Adam Petcher <adam.petcher at 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());
>>> + }
>>> +}
>
More information about the security-dev
mailing list