RFR: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93) [v3]

[Alexey Bakhtin]

On Sat, 8 May 2021 00:21:54 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add Cache.pull method
>
> src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line 377:
> 
>> 375:                     // If we are keeping state, see if the identity is in the cache
>> 376:                     if (requestedId.identity.length == SessionId.MAX_LENGTH) {
>> 377:                         s = sessionCache.pull(requestedId.identity);
> 
> Would you please add a comment here so as we know why a pull method could be used here?  For example:
> 
> 
> -                    // If we are keeping state, see if the identity is in the cache
> +                    // If we are keeping state, see if the identity is in the
> +                    // cache.  Note that for TLS 1.3, we would also clean
> +                    // up the cached session if it is not rejoinable.

Thank you, added comments as suggested.

> src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 183:
> 
>> 181:         if (id != null)
>> 182:             return sessionCache.pull(new SessionId(id));
>> 183:         return null;
> 
> As this is an internal method, it should be safe to assume that the id is non-null.  I'm fine if you want to keep the non-null check, but please use braces for if-statement (see also https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449).

Thank you. Added braces but I'd like to keep non-null check.

> src/java.base/share/classes/sun/security/util/Cache.java line 430:
> 
>> 428:             return null;
>> 429:         }
>> 430:         V value;
> 
> I may add a blank line before this line.

Thank you, added

> src/java.base/share/classes/sun/security/util/Cache.java line 442:
> 
>> 440:         entry.invalidate();
>> 441:         return value;
>> 442:     }
> 
> I may adjust the lines a little bit so as to avoid duplicated operations (see the implementation code of isValid()).
> 
> 
>          long time = (lifetime == 0) ? 0 : System.currentTimeMillis();
>          if (entry.isValid(time)) {
>               V value = entry.getValue();
>               entry.invalidate();
>               return value;
>           } else {
>               if (DEBUG) {
>                   System.out.println("Ignoring expired entry");
>               }
>               return null;
>           }

I'd like to keep my code as-is. We still need invalidate() if entry is not valid (see remove() operation).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3664