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 <neil.richa...@ngmr.net>, <neil_richa...@uk.ibm.com> # 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: <neil.richa...@ngmr.net> 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 <neil.richa...@ngmr.net>, <neil_richa...@uk.ibm.com> + */ + +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."); + } +} + +
