Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8174729

Race Condition in java.lang.reflect.WeakCache

    Details

      Backports

        Description

        Race can occur between Proxy.getProxyClass and Proxy.isProxyClass

          Issue Links

            Activity

            Hide
            adinn Andrew Dinn added a comment -
            java.lang.reflect.Proxy uses class WeakCache to retain details of Proxy classes associated with a given class loader. WeakCache uses a two-tier Map to implement the cache. The first tier map employs keys which contain a weak-reference to the classloader. The associated value is also a map. Subkeys indexing this secondary map are built using the first tier key and the collection of interfaces (Class[]) which uniquely identify the required proxy. Values in the secondary map implement a Supplier interface which allows the relevant proxy Class to be produced on demand.

            WeakCache also maintains a reverse map which maps every secondary map value to Boolean.TRUE. The reverse map is used to implement Proxy.isProxyClass. It allows clients holding a reference to a putative proxy Class to check whether the class really is a proxy.

            Implementations for values stored in the secondary map are either a CacheValue or a Factory. Initially threads attempting to obtain a proxy Class call Proxy.getProxyClass. Cincurrent calls are dealt with by racing to install a factory in the secondary map (using putIfAbsent) and then call its synchronized get method. n.b. the factory is not entered into the reverse map. Factory.get method creates a proxy, replaces the entry for itself in the secondary map with a CacheValue. The get method for CacheValue is not synchronized.

            The critical issue is the order of insertion of the CacheValue into the secondary map and reverse map. At present Factory.get calls map.replace to install the CacheValue into the secondary map before calling map.put to install the CacheValue in the reverse map. A thread suspend between these two operations allows some other thread to lookup the proxy class (e.g. by calling Proxy.resolveClass), retrieve a class cl which passes the test cl instanceof Proxy yet fails the check Proxy.isProxyClass(cl). This precise sequence of tests happens when deserializing proxy instances and leads to an InvalidClassException with message "Not a proxy".

            A simple fix for this issue is to reverse the order of the secondary map replace and the reverse map put.

            n.b. A test case to manifest this issue and validate the fix has been provided by Peter Levart.
            Show
            adinn Andrew Dinn added a comment - java.lang.reflect.Proxy uses class WeakCache to retain details of Proxy classes associated with a given class loader. WeakCache uses a two-tier Map to implement the cache. The first tier map employs keys which contain a weak-reference to the classloader. The associated value is also a map. Subkeys indexing this secondary map are built using the first tier key and the collection of interfaces (Class[]) which uniquely identify the required proxy. Values in the secondary map implement a Supplier interface which allows the relevant proxy Class to be produced on demand. WeakCache also maintains a reverse map which maps every secondary map value to Boolean.TRUE. The reverse map is used to implement Proxy.isProxyClass. It allows clients holding a reference to a putative proxy Class to check whether the class really is a proxy. Implementations for values stored in the secondary map are either a CacheValue or a Factory. Initially threads attempting to obtain a proxy Class call Proxy.getProxyClass. Cincurrent calls are dealt with by racing to install a factory in the secondary map (using putIfAbsent) and then call its synchronized get method. n.b. the factory is not entered into the reverse map. Factory.get method creates a proxy, replaces the entry for itself in the secondary map with a CacheValue. The get method for CacheValue is not synchronized. The critical issue is the order of insertion of the CacheValue into the secondary map and reverse map. At present Factory.get calls map.replace to install the CacheValue into the secondary map before calling map.put to install the CacheValue in the reverse map. A thread suspend between these two operations allows some other thread to lookup the proxy class (e.g. by calling Proxy.resolveClass), retrieve a class cl which passes the test cl instanceof Proxy yet fails the check Proxy.isProxyClass(cl). This precise sequence of tests happens when deserializing proxy instances and leads to an InvalidClassException with message "Not a proxy". A simple fix for this issue is to reverse the order of the secondary map replace and the reverse map put. n.b. A test case to manifest this issue and validate the fix has been provided by Peter Levart.
            Hide
            adinn Andrew Dinn added a comment -
            This issue is not applicable to JDK9 where the proxy caching code has been completely rewritten in order to support modules.
            Show
            adinn Andrew Dinn added a comment - This issue is not applicable to JDK9 where the proxy caching code has been completely rewritten in order to support modules.
            Hide
            adinn Andrew Dinn added a comment -
            This bug appears to be at the root of a prior JIRA relating to deserialization which was dismissed as relating simply to a reachability problem

              https://bugs.openjdk.java.net/browse/JDK-8087168

            I have a reproducer based on deserialization which led me to identify this fix. The "Not a proxy" issue it reveals is removed by this fix. However, with that fix this second reproducer generates some very arbitrary NullPointerException occurences, whose backtraces show them coming from lines of code which have no associated dereference operations. That's a very strange occurence and suggests that there is indeed something else wrong in the Proxy or Serialization code.
            Show
            adinn Andrew Dinn added a comment - This bug appears to be at the root of a prior JIRA relating to deserialization which was dismissed as relating simply to a reachability problem    https://bugs.openjdk.java.net/browse/JDK-8087168 I have a reproducer based on deserialization which led me to identify this fix. The "Not a proxy" issue it reveals is removed by this fix. However, with that fix this second reproducer generates some very arbitrary NullPointerException occurences, whose backtraces show them coming from lines of code which have no associated dereference operations. That's a very strange occurence and suggests that there is indeed something else wrong in the Proxy or Serialization code.
            Hide
            adinn Andrew Dinn added a comment - - edited
            Peter Levart provided the attached reproducer (ProxyRace.java) for the race condition. It is configured to run for 5 seconds and when run on a desktop machine it is very consistent in manifesting the error on the old build. It never manifests it on the patched build.

            It might make a useful regression test with the proviso that the test may intermittently fail to detect the bug in the unpatched code depending on timing of execution.
            Show
            adinn Andrew Dinn added a comment - - edited Peter Levart provided the attached reproducer (ProxyRace.java) for the race condition. It is configured to run for 5 seconds and when run on a desktop machine it is very consistent in manifesting the error on the old build. It never manifests it on the patched build. It might make a useful regression test with the proviso that the test may intermittently fail to detect the bug in the unpatched code depending on timing of execution.
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/9bc2f86c5e88
            User: mchung
            Date: 2017-02-28 17:01:23 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/9bc2f86c5e88 User: mchung Date: 2017-02-28 17:01:23 +0000
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/9bc2f86c5e88
            User: robm
            Date: 2017-03-22 02:08:57 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/9bc2f86c5e88 User: robm Date: 2017-03-22 02:08:57 +0000

              People

              • Assignee:
                rpatil Ramanand Patil
                Reporter:
                adinn Andrew Dinn
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: