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.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());
>>> + }
>>> +}
>
