7u Code review request for 7033170, 7092821, 7092825

Sean Mullan sean.mullan at oracle.com
Fri Aug 7 13:16:20 UTC 2015


Hi David,

On 08/07/2015 08:44 AM, David Schlosnagle wrote:
> I forgot to mention one other workaround for this bug, as used in Guava
> is cloning the instance if clone() is supported, see
> https://github.com/google/guava/issues/1197
>
> On Friday, August 7, 2015, David Schlosnagle <schlosna at gmail.com
> <mailto:schlosna at gmail.com>> wrote:
>
>     // reviving this ghost
>
>     Hi Valerie, Sean, and sec-dev,
>
>     I am curious if there has been any movement on incorporating a fix
>     for https://bugs.openjdk.java.net/browse/JDK-7092821? I have
>     encountered several systems where this is a significant contention
>     and scale bottleneck. While there are some workarounds such as
>     pooling and reusing Provider instances, that seems like a band-aid,
>     and fixing the JDK is a better path.

I don't know if Valerie has made any progress on this issue, so I'll let 
her add any details.

>
>     If I were to put a webrev together, would someone be kind enough
>     to sponsor it for me?

Yes, I think we could do that and I see you have already signed the OCA.

> Are there any existing open source load tests
>     to verify these changes?

Not that I know of. It would be great if you could implement a JMH 
microbenchmark as part of your fix.

> Is there a good mechanism in OpenJDK now to
>     run JMH across all of the supported platforms (I only have access to
>     a small subset of the permutation of platforms).

That's being worked on for JDK 9 but AFAIK it isn't ready yet. See 
http://openjdk.java.net/jeps/230

Thanks for your help!

--Sean

>
>     Thanks!
>
>     - Dave
>
>     On Thursday, January 12, 2012, Valerie (Yu-Ching) Peng
>     <valerie.peng at oracle.com
>     <javascript:_e(%7B%7D,'cvml','valerie.peng at oracle.com');>> wrote:
>
>         Dave,
>         Thanks for the comments.
>         Let me think about it some more and see how to better address
>         this kind of racing issue.
>
>         If you have concerns on fixes for 7033170 and 7092825, please
>         let me know.
>
>         Sean,
>         Can you please ignore the review request for 7092821 for now.
>         I'll send out an updated version later.
>         If you can still review the remaining two, that'd be great.
>
>         Thanks,
>         Valerie
>
>         On 01/11/12 21:48, David Schlosnagle wrote:
>
>             On Wed, Jan 11, 2012 at 8:04 PM, Valerie (Yu-Ching) Peng
>             <valerie.peng at oracle.com>  wrote:
>
>                 7092821: java.security.Provider.getService() is
>                 synchronized and became scalability bottleneck.
>                 jdk7u Webrev:
>                 http://cr.openjdk.java.net/~valeriep/7092821_7u/
>                 jdk8 Webrev: http://cr.openjdk.java.net/~valeriep/7092821/
>
>             Valerie,
>
>             You might already be aware of this, but there is a data race
>             on lines
>             685 - 686 of the Provider's getService(String, String)
>             method. If
>             there are concurrent callers to getService while lookupCache
>             == null,
>             the lookupCache may be overwritten by a new
>             ConcurrentHashMap after
>             another thread has just instantiated and  populated an entry
>             in the
>             cache leading to thrashing on lookupCache. It might be
>             worthwhile to
>             either use a double checked lock to avoid the race at the
>             expense of
>             an additional lock and volatile read in the case lookupCache
>             == null
>             or add a comment indicating that this is an accepted data race.
>
>             There is also now the possibility that if one thread is
>             executing
>             getService while another thread invokes one of the methods
>             that sets
>             lookupCache = null, there could then be a
>             NullPointerException on line
>             703 as the volatile read would now see a null and fail. You
>             could
>             prevent that by either moving the putIfAbsent under the lock
>             (not
>             ideal for performance if you're already seeing contention),
>             or just
>             maintain a local reference to the cache map and use it
>             throughout the
>             getService method. This would mean that if the lookupCache
>             reference
>             was concurrently mutated, the putIfAbsent would basically be
>             a write
>             to the local map reference which is now garbage, but
>             shouldn't really
>             hurt anything.
>
>             I'd propose something along the lines of the following to
>             address this:
>
>                   public Service getService(String type, String algorithm) {
>                       ServiceKey key = new ServiceKey(type, algorithm);
>                       ConcurrentMap<ServiceKey,Service>
>             localLookupCache = getLookupCache();
>                       Service result = localLookupCache.get(key);
>                       if (result != null) {
>                           return (result == NULL_MARK? null : result);
>                       }
>                       synchronized (this) {
>                           checkInitialized();
>                           if (serviceMap != null) {
>                               result = serviceMap.get(key);
>                           }
>                           if (result == null) {
>                               ensureLegacyParsed();
>                               result = (legacyMap != null) ?
>             legacyMap.get(key) : null;
>                           }
>                       }
>                       // under concurrent mutation of lookupCache, this
>             will write
>             to map that is no
>                       // longer the active cache
>                       localLookupCache.putIfAbsent(key, (result == null?
>             NULL_MARK : result));
>                       return result;
>                   }
>
>                   private ConcurrentMap<ServiceKey,Service>
>             getLookupCache() {
>                       if (lookupCache == null) {
>                           synchronized (this) {
>                               // must fall back on double checked lock
>                               if (lookupCache == null) {
>                                   lookupCache = new ConcurrentHashMap<>();
>                               }
>                           }
>                       }
>                       return lookupCache;
>                   }
>
>
>             - Dave
>
>
>             For reference, here were the original changes:
>                429     // Cache for service lookups. Discard whenever
>             services are changed.
>                430     private transient volatile
>             ConcurrentMap<ServiceKey,Service>
>             lookupCache;
>             ...snip...
>                682     public Service getService(String type, String
>             algorithm) {
>                683         ServiceKey key = new ServiceKey(type, algorithm);
>                684         Service result = null;
>                685         if (lookupCache == null) {
>                686             lookupCache = new ConcurrentHashMap<>();
>                687         } else {
>                688             result = lookupCache.get(key);
>                689             if (result != null) {
>                690                 return (result == NULL_MARK? null :
>             result);
>                691             }
>                692         }
>                693         synchronized (this) {
>                694             checkInitialized();
>                695             if (serviceMap != null) {
>                696                 result = serviceMap.get(key);
>                697             }
>                698             if (result == null) {
>                699                 ensureLegacyParsed();
>                700                 result = (legacyMap != null) ?
>             legacyMap.get(key) : null;
>                701             }
>                702         }
>                703         lookupCache.putIfAbsent(key, (result == null?
>             NULL_MARK : result));
>                704         return result;
>                705     }
>
>



More information about the security-dev mailing list