RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
Kazunori Ogata
OGATAK at jp.ibm.com
Tue Oct 17 11:48:49 UTC 2017
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
More information about the core-libs-dev
mailing list