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

[Kazunori Ogata]

Hi Peter,

Thank you for your review and summarizing the change.

Regards,
Ogata


Peter Levart <peter.levart at gmail.com> wrote on 2017/10/24 14:11:01:

> From: Peter Levart <peter.levart at gmail.com>
> To: Kazunori Ogata <OGATAK at jp.ibm.com>
> Cc: Alan Bateman <Alan.Bateman at oracle.com>, 
core-libs-dev at openjdk.java.net
> Date: 2017/10/24 14:11
> Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in 
> ObjectInputStream.readObject()
> 
> Hi,
> 
> I think that what Ogata has in webrev.03 is correct and the reasoning 
> could be as follows:
> 
> - each thread writes to field 'cachedLoader' only from its own set of 
> values that are distinct from the sets of values that may be written by 
> any other thread, except null value.
> - each thread reads field 'cachedLoader' and can verfy that it has read 
a 
> value that belongs to the set of its own written values, or null value 
(in
> other words, a thread can verify that it has read a value that was 
written
> by self or null value).
> - reads and writes of own set of non-null values always appear in 
program 
> order since they are performed by same thread
> - a sequence of writes of own set of non-null values performed by some 
> thread begins after this thread 1st observes a null value read from the 
> field and ends before this thread finally writes null value back to the 
> field. The last write performed by some thread is therefore always a 
write
> of null value.
> 
> No matter how writes performed by a mixture of threads finally hit the 
> actual field, since each thread that writes to it issues its final write 

> of null value, the value that eventually ends in the field is null 
value.
> 
> Does this make sense?
> 
> Regards, Peter

> On 10/23/17 22:47, Peter Levart wrote:
> 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
> 
> 
> 
>