Neil, Hi. For background, I filed 6597112. Thanks for looking into it.
I don't see any correctness flaws with your suggested fix. It feels to me, however, unnecessarily elaborate for the problem being fixed. It adds an additional (and less localized) use of the Target.pinImpl/unpinImpl mechanism, which is currently only used to "pin" remote objects that are known to have live remote references (or for the internal "permanent" case), for which ongoing strong reachability from a static root (i.e. ObjectTable) is necessary. For this bug, it is only necessary to ensure strong reachability during the execution of the UnicastServerRef.exportObject method in the exporting thread. The reachabilityFence method from the concurrency folks' Fences proposal would enable a robust one-line fix: http://g.oswego.edu/dl/jsr166/dist/docs/java/util/concurrent/atomic/Fences.html#reachabilityFence(java.lang.Object) But I haven't been following the status of that API making it into the JDK. Barring that, other localized approaches should work-- the canonical if not optimal example is passing the reference to a native method. It would be nice to have a recommended idiom from JVM experts.[*] Another possibility, though, is to just not throw this ExportException (which is effectively an assertion failure) in ObjectTable.putTarget, but rather let the remote object's export appear to succeed even though it is known to have been garbage collected. Given that the caller isn't ensuring strong reachability of the object, the effect would be indistinguishable from the remote object being collected immediately after the remote object export completes anyway. [This likely indicates a bug in a real application, as opposed to a test case, which is why 6597112 didn't seem especially severe, unlike 6181943.] With this approach, it would be important to guard against the case of ObjectTable.Reaper processing a WeakRef from the queue before ObjectTable.putTarget has executed for it. Regarding this diff in particular: > - * If "permanent" is true, then the impl is pinned permanently > - * (the impl will not be collected via distributed and/or local > - * GC). If "on" is false, than the impl is subject to > - * collection. Permanent objects do not keep a server from > - * exiting. > + * Upon return, the impl is pinned, thus not initially eligible > + * for collection via distributed and/or local GC). > + * If "permanent" is false, the impl subsequently may be made subject to > + * collection by calling {@link #unpinImpl()}. > + * Permanent or pinned objects do not keep a server from exiting. I would leave the last sentence alone-- pinned objects can prevent JVM exit, the point there is really that so-called "permanent" objects don't. Cheers, -- Peter [*] The fix for the similar bug 6181943, in StreamRemoteCall.executeCall, used a DGCAckHandler instance to hold references to objects to be kept strongly reachable for the duration of a remote call, and the invocation of "ackHandler" in the finally block ensured that the container itself remains reachable from the thread for the same duration. For 6181943, there was a desire to hold only remote references, not the entirety of the call arguments, hence the use of DGCAckHandler which cooperates with the object output stream. The 6597112 case is simpler: just need to hold the remote object being exported. On Mar 24, 2011, at 4:54 PM, Neil Richards wrote: > Hi all, > Can I encourage further review / comment on my suggested fix (and > testcase) for this problem [1] [2], please ? > > For reference, I include the 'hg export' for the latest version of the > change below, > > Any feedback gratefully received, > Thanks, > Neil > > PS: I see the bug details [2] are not currently externally visible, but > don't know of a good reason for this. Could this be reviewed, please? > > [1] > http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-March/006144.html > [2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6597112 > > > -- > Unless stated above: > IBM email: neil_richards at uk.ibm.com > IBM United Kingdom Limited - Registered in England and Wales with number > 741598. > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > > > # HG changeset patch > # User Neil Richards <[email protected]>, <[email protected]> > # Date 1298369045 0 > # Branch j6597112 > # Node ID 0c22e02a9016bd340e2182e96a10fe96cd75c8ac > # Parent 6bbc7a4734952ae7604578f270e1566639fa8752 > 6597112: referential integrity loophole during remote object export > Summary: Eliminate weak ref GC window during RMI export > Contributed-by: <[email protected]> > > diff --git a/src/share/classes/sun/rmi/server/UnicastServerRef.java > b/src/share/classes/sun/rmi/server/UnicastServerRef.java > --- a/src/share/classes/sun/rmi/server/UnicastServerRef.java > +++ b/src/share/classes/sun/rmi/server/UnicastServerRef.java > @@ -204,7 +204,11 @@ > > Target target = > new Target(impl, this, stub, ref.getObjID(), permanent); > - ref.exportObject(target); > + try { > + ref.exportObject(target); > + } finally { > + target.unpinImpl(); > + } > hashToMethod_Map = hashToMethod_Maps.get(implClass); > return stub; > } > diff --git a/src/share/classes/sun/rmi/transport/Target.java > b/src/share/classes/sun/rmi/transport/Target.java > --- a/src/share/classes/sun/rmi/transport/Target.java > +++ b/src/share/classes/sun/rmi/transport/Target.java > @@ -77,11 +77,11 @@ > * Construct a Target for a remote object "impl" with > * a specific object id. > * > - * If "permanent" is true, then the impl is pinned permanently > - * (the impl will not be collected via distributed and/or local > - * GC). If "on" is false, than the impl is subject to > - * collection. Permanent objects do not keep a server from > - * exiting. > + * Upon return, the impl is pinned, thus not initially eligible > + * for collection via distributed and/or local GC). > + * If "permanent" is false, the impl subsequently may be made subject to > + * collection by calling {@link #unpinImpl()}. > + * Permanent or pinned objects do not keep a server from exiting. > */ > public Target(Remote impl, Dispatcher disp, Remote stub, ObjID id, > boolean permanent) > @@ -114,9 +114,6 @@ > } > > this.permanent = permanent; > - if (permanent) { > - pinImpl(); > - } > } > > /** > @@ -212,7 +209,7 @@ > * can be garbage collected locally. But only if there the refSet > * is empty. All of the weak/strong handling is in WeakRef > */ > - synchronized void unpinImpl() { > + public synchronized void unpinImpl() { > /* only unpin if: > * a) impl is not permanent, and > * b) impl is not already unpinned, and > diff --git a/src/share/classes/sun/rmi/transport/WeakRef.java > b/src/share/classes/sun/rmi/transport/WeakRef.java > --- a/src/share/classes/sun/rmi/transport/WeakRef.java > +++ b/src/share/classes/sun/rmi/transport/WeakRef.java > @@ -33,7 +33,7 @@ > * > * This class extends the functionality of java.lang.ref.WeakReference in > * several ways. The methods pin() and unpin() can be used to set > - * whether the contained reference is strong or weak (it is weak upon > + * whether the contained reference is strong or weak (it is strong upon > * construction). The hashCode() and equals() methods are overridden so > * that WeakRef objects hash and compare to each other according to the > * object identity of their referents. > @@ -51,18 +51,24 @@ > > /** > * Create a new WeakRef to the given object. > + * The given object is initially pinned, ie. referenced strongly. > + * It may be subsequently unpinned by calling {@link #unpin()}. > */ > public WeakRef(Object obj) { > super(obj); > setHashValue(obj); // cache object's "identity" hash code > + strongRef = obj; > } > > /** > * Create a new WeakRef to the given object, registered with a queue. > + * The given object is initially pinned, ie. referenced strongly. > + * It may be subsequently unpinned by calling {@link #unpin()}. > */ > public WeakRef(Object obj, ReferenceQueue q) { > super(obj, q); > setHashValue(obj); // cache object's "identity" hash code > + strongRef = obj; > } > > /** > diff --git > a/test/java/rmi/server/UnicastRemoteObject/gcDuringExport/GcDuringExport.java > b/test/java/rmi/server/UnicastRemoteObject/gcDuringExport/GcDuringExport.java > new file mode 100644 > --- /dev/null > +++ > b/test/java/rmi/server/UnicastRemoteObject/gcDuringExport/GcDuringExport.java > @@ -0,0 +1,67 @@ > +/* > + * Copyright (c) 2011 Oracle and/or its affiliates. All rights reserved. > + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 only, as > + * published by the Free Software Foundation. > + * > + * This code is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * version 2 for more details (a copy is included in the LICENSE file that > + * accompanied this code). > + * > + * You should have received a copy of the GNU General Public License version > + * 2 along with this work; if not, write to the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. > + * > + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA > + * or visit www.oracle.com if you need additional information or have any > + * questions. > + */ > + > +/* > + * Portions Copyright (c) 2011 IBM Corporation > + */ > + > +/* > + * @test > + * @bug 6597112 > + * @summary Objects should not be GC-able as they are being exported to RMI > + * @author Neil Richards <[email protected]>, <[email protected]> > + */ > + > +import java.rmi.Remote; > +import java.rmi.server.UnicastRemoteObject; > + > +public class GcDuringExport { > + private static final long MAX_EXPORT_ITERATIONS = 50000; > + > + public static void main(String[] args) throws Exception { > + final Thread gcInducingThread = new Thread() { > + public void run() { while (true) { > + System.gc(); > + try { Thread.sleep(1); } catch (InterruptedException e) > { } > + } > + } > + }; > + > + gcInducingThread.setDaemon(true); > + gcInducingThread.start(); > + > + long i = 0; > + try { > + while (i < MAX_EXPORT_ITERATIONS) { > + i++; > + UnicastRemoteObject.exportObject(new Remote() { }, 0); > + } > + } catch (Throwable e) { > + throw new RuntimeException("Test FAILED on iteration " + i + > ".", e); > + } > + > + System.out.println("Test successfully exported " + i + " objects."); > + } > +} > + > + > >
