[PATCH] 8165751: NPE in Signature with java.security.debug=provider
Sean Mullan
sean.mullan at oracle.com
Tue Dec 13 16:56:29 UTC 2016
On 12/13/16 11:30 AM, Adam Petcher wrote:
> Thanks for the review.
>
> After thinking about this some more, I don't like the idea of using
> Optional for a member variable due to the limitations of this class and
> the lack of support for this usage of it. I'll send out a new diff that
> uses a standard null check.
>
> See below for other comments.
>
>
> On 12/12/2016 9:20 PM, Wang Weijun wrote:
>> Hi Adam
>>
>> The only behavior change is with the debug output, right?
> Yes. This will be more obvious in my next diff.
>>
>> 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().
> It doesn't print the "from: " when there is no provider. So the string
> will look like "Signature.DSA verification algorithm" in this case. I
> don't have a strong opinion on whether we should print "from: no
> provider" (for example), but Sean expressed a preference for leaving
> this entire part blank when there is no provider (as it is in my last
> diff).
Maybe "from: (no provider)" would be better. I think a previous version
had "from: none", but thinking about it more, "from: (no provider)" is
probably better than not printing anything for a null provider.
--Sean
>>
>> 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