[PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Sean Mullan]

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