RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
Hi all, Please review a change for JDK-8188858. Bug report: https://bugs.openjdk.java.net/browse/JDK-8188858 Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.00/ This change caches the result of latestUserDefinedLoader() when objects are deserialized, so the decerializer can avoid redundant stack walking to resolve classes of deserializing objects. This change improved maxjOPS of SPECjbb2015 by 6% on a POWER8 machine. Regards, Ogata
On 06/10/2017 11:34, Kazunori Ogata wrote:
Hi all,
Please review a change for JDK-8188858.
Bug report: https://bugs.openjdk.java.net/browse/JDK-8188858 Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.00/
This change caches the result of latestUserDefinedLoader() when objects are deserialized, so the decerializer can avoid redundant stack walking to resolve classes of deserializing objects. Some of the bugs/abuses of OIS come about from calling it on different threads with different contexts. So I think this optimization can only work if to confine it to the thread calling readUnshared, meaning readResolve cannot skip latestUserDefinedLoader() when called on other threads.
-Alan
Hi Alan, Thank you for your comment. I agree that the current code is not thread safe, but I think OIS itself is not thread safe either. The issue you pointed out occurs when two threads calls readObject()/readUnshared() simultaneously, and the result of such situation is undefined in any way in my understanding. Do we need to ensure the same behavior for such an error case? Regarding readResolve, I think readResolve() won't use the cached loader because readResolve() of a deserialized class can't call OIS.resolveClass(). Regards, Ogata Alan Bateman <Alan.Bateman@oracle.com> wrote on 2017/10/09 20:24:20:
From: Alan Bateman <Alan.Bateman@oracle.com> To: Kazunori Ogata <OGATAK@jp.ibm.com>, core-libs-dev@openjdk.java.net Date: 2017/10/09 20:24 Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
On 06/10/2017 11:34, Kazunori Ogata wrote:
Hi all,
Please review a change for JDK-8188858.
Bug report: https://bugs.openjdk.java.net/browse/JDK-8188858 Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.00/
This change caches the result of latestUserDefinedLoader() when objects are deserialized, so the decerializer can avoid redundant stack walking to resolve classes of deserializing objects. Some of the bugs/abuses of OIS come about from calling it on different threads with different contexts. So I think this optimization can only work if to confine it to the thread calling readUnshared, meaning readResolve cannot skip latestUserDefinedLoader() when called on other threads.
-Alan
On 10/10/2017 10:50, Kazunori Ogata wrote:
Hi Alan,
Thank you for your comment.
I agree that the current code is not thread safe, but I think OIS itself is not thread safe either. The issue you pointed out occurs when two threads calls readObject()/readUnshared() simultaneously, and the result of such situation is undefined in any way in my understanding. Do we need to ensure the same behavior for such an error case? OIS is very interesting to attackers so you will need to take deliberate abuses of the API into account. I realize it's a pain but it's one of the reasons why we have to be cautious about optimizations in this area.
-Alan
Hi Alan, Thank you for your comment. I was not fully aware of the possibility of attacking... I updated the patch to check if the current thread is the same as the thread cached the loader. Updated webreb: http://cr.openjdk.java.net/~horii/8188858/webrev.01/ Regards, Ogata From: Alan Bateman <Alan.Bateman@oracle.com> To: Kazunori Ogata <OGATAK@jp.ibm.com> Cc: core-libs-dev@openjdk.java.net Date: 2017/10/10 21:41 Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject() On 10/10/2017 10:50, Kazunori Ogata wrote:
Hi Alan,
Thank you for your comment.
I agree that the current code is not thread safe, but I think OIS itself is not thread safe either. The issue you pointed out occurs when two threads calls readObject()/readUnshared() simultaneously, and the result of such situation is undefined in any way in my understanding. Do we need to ensure the same behavior for such an error case? OIS is very interesting to attackers so you will need to take deliberate abuses of the API into account. I realize it's a pain but it's one of the reasons why we have to be cautious about optimizations in this area.
-Alan
On 12/10/2017 07:07, Kazunori Ogata wrote:
Hi Alan,
Thank you for your comment. I was not fully aware of the possibility of attacking...
I updated the patch to check if the current thread is the same as the thread cached the loader.
Updated webreb: http://cr.openjdk.java.net/~horii/8188858/webrev.01/
This is better but it still not safe. You'll have to atomically set/get the cachedLoader or put it into a thread local to ensure that resolveClass picks up the loader cached by the current thread. A thread local could work too although (needs study) it might need a reference to the OIS to guard against nested deserialization with a different stream. -Alan
Hi Alan, Alan Bateman <Alan.Bateman@oracle.com> wrote on 2017/10/12 22:17:48:
This is better but it still not safe. You'll have to atomically set/get the cachedLoader or put it into a thread local to ensure that resolveClass picks up the loader cached by the current thread. A thread local could work too although (needs study) it might need a reference to
the OIS to guard against nested deserialization with a different stream.
Thank you for your review. I fixed the code that does not read the cachedLoader atomically when the code tries to update it. Updated webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.02/ The updated code does not use atomic CAS or ThreadLocal. This code can race when the cachedLoader is null, but I think it works correctly because a pair of a thread and a class loader (stored in a CachedLoader) is always correct regardless of the value of the cachedLoader field, and the thread that writes the cachedLoader last resets it to null. The thread that failed to cache a loader simply calls latestUserDefinedLoader(), which is the same behavior as the original code. Considering that multi-thread use of an OIS is rare, I don't want to add an overhead of CAS to update the cachedLoader, especially on the POWER platform because CAS has high overhead. Caching loaders in a ThreadLocal in each OIS can be more thread safe, but I'm not sure if its memory overhead is worth doing so for the rare (and correctly working) case. Regards, Ogata
Hi Ogata, I think that webrev.02 is safe as you describe. But there's one case which now has some overhead. It's a common practice to subclass ObjectInputStream and override resloveClass() method and implement custom resolving (without calling super.resolveClass()). In such case, the cached loader is unnecessarily set-up and then not used. So I'm thinking about lazy caching. For example: - let public readObject() / readUnshared() entry and exit points just clear the cached loader (set it to null). - let resloveClass() do something like the following at entry: CachedLoader cl = cachedLoader; Thread curThread = Thread.currentThread(); ClassLoader loader; if (cl == null) { loader = latestUserDefinedLoader(); cl = 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); What do you think? Regards, Peter On 10/13/2017 02:36 PM, Kazunori Ogata wrote:
Hi Alan,
Alan Bateman <Alan.Bateman@oracle.com> wrote on 2017/10/12 22:17:48:
This is better but it still not safe. You'll have to atomically set/get the cachedLoader or put it into a thread local to ensure that resolveClass picks up the loader cached by the current thread. A thread local could work too although (needs study) it might need a reference to the OIS to guard against nested deserialization with a different stream. Thank you for your review. I fixed the code that does not read the cachedLoader atomically when the code tries to update it.
Updated webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.02/
The updated code does not use atomic CAS or ThreadLocal. This code can race when the cachedLoader is null, but I think it works correctly because a pair of a thread and a class loader (stored in a CachedLoader) is always correct regardless of the value of the cachedLoader field, and the thread that writes the cachedLoader last resets it to null. The thread that failed to cache a loader simply calls latestUserDefinedLoader(), which is the same behavior as the original code.
Considering that multi-thread use of an OIS is rare, I don't want to add an overhead of CAS to update the cachedLoader, especially on the POWER platform because CAS has high overhead. Caching loaders in a ThreadLocal in each OIS can be more thread safe, but I'm not sure if its memory overhead is worth doing so for the rare (and correctly working) case.
Regards, Ogata
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(). Regards, Peter
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): http://cr.openjdk.java.net/~plevart/jdk10-dev/8188858_OIS.latestUserDefinedL... Regards, Peter
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@gmail.com> To: Kazunori Ogata <OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@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_-7Ep... Regards, Peter
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@gmail.com> To: Kazunori Ogata <OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@oracle.com> Cc: core-libs-dev@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_-7Ep...
Regards, Peter
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@gmail.com> To: Kazunori Ogata<OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@oracle.com> Cc:core-libs-dev@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_-7Ep...
Regards, Peter
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@gmail.com> To: Kazunori Ogata<OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@oracle.com> Cc:core-libs-dev@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_-7Ep...
Regards, Peter
participants (4)
-
Alan Bateman
-
David Holmes
-
Kazunori Ogata
-
Peter Levart