On Thu, 2 Sep 2021 11:35:52 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted >> `MethodHandle` and it can introduce a class loader leak through its >> `MethodType`. >> >> Proposed fix introduces a 2-level cache (1 element each) where 1st level can >> only contain `MethodHandle`s which are guaranteed to not introduce any >> dependencies on new class loaders compared to the original `MethodHandle`. >> 2nd level is backed by a `SoftReference` and is used as a backup when the >> result of `MethodHandle.asType()` conversion can't populate the higher level >> cache. >> >> The fix is based on [the >> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/) >> made by Peter Levart @plevart back in 2015. >> >> Testing: tier1 - tier6 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 877: > 875: } > 876: if (asTypeSoftCache != null) { > 877: atc = asTypeSoftCache.get(); NPE is possible here too! asTypeSoftCache is a non-volatile field which is read twice. First time in the if (...) condition, 2nd time in the line that de-references it to call .get(). This is a data-race since concurrent thread may be setting this field from null to non-null. Those two reads may get reordered. 1st read may return non-null while 2nd may return null. This can be avoided if the field is read just once by introducing a local variable to store its value. src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 878: > 876: if (asTypeSoftCache != null) { > 877: atc = asTypeSoftCache.get(); > 878: if (newType == atc.type) { NPE is possible here! act can be null as it is a result of SoftReference::get src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 933: > 931: } > 932: > 933: /* Returns true when {@code loader} keeps {@code mt} either directly > or indirectly through the loader delegation chain. */ Well, to be precise, loader can't keep mt alive. It would be better to say "keeps mt components alive" ... src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 948: > 946: if (isBuiltinLoader(defLoader)) { > 947: return true; // built-in loaders are always reachable > 948: } No need for special case here. isAncestorLoaderOf(defLoader, loader) already handles this case. ------------- PR: https://git.openjdk.java.net/jdk/pull/5246
