RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()

[Peter Levart]

Hi Ogata,

Sorry for late reply. You are absolutely right. Good catch! I missed 
this scenario. The criteria for placing the mark (current Thread) on a 
cachedLoader must include the check that validates previous value for 
later restoration which uses the same criteria. Only in such case will 
one thread never restore something that has not been placed by it. And 
this guarantees that consumed OIS will never retain a reference to any 
ClassLoader or Thread. Your webrev.03 looks good to me.

Regards, Peter

On 10/17/17 13:48, Kazunori Ogata wrote:
> Hi Peter,
>
> Thank you for your comments and the fix.  It's a good idea to mark
> cachedLoader with the Thread object.
>
>
> I think we need to check the marking thread of cachedLoader before
> updating it.  Otherwise, there is a scenario that can leak a CachedLoader
> object:
>
> //1. Thread-A enters readObject() and then call resolveClass()
> outerCL-A <- null
> cachedLoader <- Thread-A
> cachedLoader <- CachedLoader-A
>
> //2. Thread-B enters readObject() and then call resolveClass()
> outerCL-B <- CachedLoader-A
> cachedLoader <- Thread-B
> cachedLoader <- CachedLoader-B1
>
> //3. Thread-B returns from readObject()
> cachedLoader is unchanged because outerCL.thread == Thread-A
>
> //4. Thread-B enters readObject() again and then call resolveClass()
> outerCL-B <- CachedLoader-B1
> cachedLoader <- Thread-B
> cachedLoader <- CachedLoader-B2
>
> //5. Thread-A returns from readObject()
> cachedLoader <- null
>
> //6. Thread-B returns from readObject()
> cachedLoader <- CachedLoader-B1  // Because outerCL-B.thread is Thread-B
>
>
> By adding checking before updating the mark, Thread-B won't update
> cachedLoader, or it only saves null when race occurs (and always restores
> to null on exit).
>
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~horii/8188858/webrev.03/
>
> I also made minor changes to reduce the number of invocation of the JNI
> method Thread.currentThread().
>
>
> Regards,
> Ogata
>
>
>
> From:   Peter Levart <peter.levart at gmail.com>
> To:     Kazunori Ogata <OGATAK at jp.ibm.com>, Alan Bateman
> <Alan.Bateman at oracle.com>
> Cc:     core-libs-dev at openjdk.java.net
> Date:   2017/10/16 19:58
> Subject:        Re: RFR: 8188858: Caching latestUserDefinedLoader()
> results in ObjectInputStream.readObject()
>
>
>
> Hi Ogata,
>
> I found a problem in my last suggestion. See below...
>
> On 10/16/2017 11:36 AM, Peter Levart wrote:
>> On 10/16/2017 11:02 AM, Peter Levart wrote:
>>> For example:
>>> - let public readObject() / readUnshared() entry and exit points just
>>> clear the cached loader (set it to null).
>> An alternative would be for entry point to save and clear the cached
>> loader while exit point would restore / clear it if it is from correct
>> thread / when the call was not nested. Like the following:
>>
>> public Object readObject() {
>>      CachedLoader outerCL = cachedLoader;
>>      cachedLoader = null;
>>      try {
>>          ...
>>      } finally {
>>          if (outerCL == null || outerCL.thread ==
>> Thread.currentThread()) {
>>              // restore/clear cached loader when nested/outer call ends
>>              cachedLoader = outerCL;
>>          }
>>      }
>> }
>>
>> with resolveClass() fragment repeated here for comparison:
>>
>>            CachedLoader cl = cachedLoader;
>>            Thread curThread = Thread.currentThread();
>>            ClassLoader loader;
>>            if (cl == null) {
>>                loader = latestUserDefinedLoader();
>>                cachedLoader = new CachedLoader(loader, curThread);
>>            } else if (cl.thread == curThread) {
>>                loader = cl.loader;
>>            } else {
>>                // multi threaded use
>>                loader = latestUserDefinedLoader();
>>            }
>>
>>            // and then...
>>            return Class.forName(name, false, loader);
>>
>>
>> There are all sorts of races possible when called concurrently from
>> multiple threads, but the worst consequence is that the loader is not
>> cached. I also think that even in the presence of races, the
>> cachedLoader is eventually cleared when all calls to OIS complete. I
>> couldn't think of a situation where such cached loader would remain
>> hanging off the completed OIS because of races.
>>
>> Well, there is one such situation but for a different reason. For
>> example, if an OIS subclass is constructed solely to override
>> resolveClass method to make it accessible to custom code (for example,
>> make it public and call super.resolveClass()) in order to provide a
>> utility for resolving classes with the default OIS semantics, but such
>> OIS instance is never used for deserialization itself
>> (readObject()/readUnshared() is never called).
>>
>> To solve this problem, resolveClass() logic, including lazy caching,
>> should be moved to a private method (resolveClass0()) with protected
>> resolveClass() treated like public readObject()/readUnshared() with
>> before/after treatment of cached loader around delegation to
>> resolveClass0(). All OIS internal uses of resolveClass() should then
>> be redirected to resolveClass0().
> Oops, this would not work for subclasses that override resolveClass()
> with custom logic. Hm...
>
> The correct and optimal solution is a little bit more involved, I think.
> Here's what I think should work (did not run any tests yet):
>
> https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Eplevart_jdk10-2Ddev_8188858-5FOIS.latestUserDefinedLoader.caching_webrev.01_&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=p-FJcrbNvnCOLkbIdmQ2tigCrcpdU77tlI2EIdaEcJw&m=PbaGqOdJOR6jMQkXDVYmjn6832m7o0LU2bzwt2awUgQ&s=gKz_rwcTcGIw8JvmRqlg1-OtjqFNXmIs4oQmIXlF3Wc&e=
>
>
>
> Regards, Peter
>
>
>
>