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

[David Holmes]

Hi Peter, Ogata,

Are you saying this is correct even if cachedLoader is not volatile? If 
so then that needs to be clearly documented.

Cheers,
David

On 24/10/2017 3:11 PM, Peter Levart wrote:
> 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
>>>
>>>
>>>
>>>
>>
>