Neil,
On Apr 8, 2011, at 1:17 PM, Neil Richards wrote:
> Hi Peter,
> Thank you for your review on this problem - it really helped in
> highlighting some invalid assumptions I was making about the way that
> the RMI code behaves.
>
> I had assumed that, once the stub is returned from exportObject, the
> 'liveness' of the stub would control the the 'liveness' of the exported
> object (as the stub is, logically, a kind of remote reference to the
> object).
>
> I'd argue this is a naturally reasonable expectation to have (ie. it
> would be good to enhance the RMI mechanism so that behaves this way),
> but, as you point out, that's not how it currently behaves (there are
> current get-out clauses in the RMI spec which warn about the behaviour
> being different than one would initially expect in this case).
Yes, this is the subject of a different, old bug:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4114579
which I agree is problematic, but that behavior having existed for so long, I
think that there is a question of whether fixing it now would cause more harm
than good. At any rate, it's separate from 6597112.
> Given all of this, your suggestion of modifying ObjectTable.putTarget()
> such that it is tolerant of being given Targets whose impl objects may
> have been GC'd (ie. without it throwing an exception) seems excellent to
> me.
>
> Please find below a (far simpler) revised suggested changeset, which
> addresses the problem in this fashion.
>
> (NB: In the new changeset, the check for the impl having been GC'd is
> now within the block synchronized on tableLock. The operation of the
> Reaper thread is also performed synchronized on this lock. This ensures
> that the keep alive count is incremented / decremented properly, even if
> the impl happens to be GC'd after the check).
>
> Please let me know if you have any further comments or suggestions on
> this,
This fix looks good to me. The synchronization subtlety may be worth a brief
comment, to explain that you don't want the Reaper processing to occur in
between the null guard and the put/increment actions.
Cheers,
-- Peter
> # HG changeset patch
> # User Neil Richards <neil.richa...@ngmr.net>, <neil_richa...@uk.ibm.com>
> # Date 1302196316 -3600
> # Branch j6597112
> # Node ID cfa1f7fcaabb03ab79aae8f051e4c084b45c75b9
> # Parent aa13e7702cd9d8aca9aa38f1227f966990866944
> 6597112: referential integrity loophole during remote object export
> Summary: Make ObjectTable.putTarget() tolerant of Target's with a GC'd impl
> Contributed-by: <neil.richa...@ngmr.net>
>
> diff -r aa13e7702cd9 -r cfa1f7fcaabb
> src/share/classes/sun/rmi/transport/ObjectTable.java
> --- a/src/share/classes/sun/rmi/transport/ObjectTable.java Tue Mar 29
> 20:19:55 2011 -0700
> +++ b/src/share/classes/sun/rmi/transport/ObjectTable.java Thu Apr 07
> 18:11:56 2011 +0100
> @@ -175,25 +175,21 @@
> DGCImpl.dgcLog.log(Log.VERBOSE, "add object " + oe);
> }
>
> - Remote impl = target.getImpl();
> - if (impl == null) {
> - throw new ExportException(
> - "internal error: attempt to export collected object");
> - }
> + synchronized (tableLock) {
> + if (null != target.getImpl()) {
> + if (objTable.containsKey(oe)) {
> + throw new ExportException(
> + "internal error: ObjID already in use");
> + } else if (implTable.containsKey(weakImpl)) {
> + throw new ExportException("object already exported");
> + }
>
> - synchronized (tableLock) {
> - if (objTable.containsKey(oe)) {
> - throw new ExportException(
> - "internal error: ObjID already in use");
> - } else if (implTable.containsKey(weakImpl)) {
> - throw new ExportException("object already exported");
> - }
> + objTable.put(oe, target);
> + implTable.put(weakImpl, target);
>
> - objTable.put(oe, target);
> - implTable.put(weakImpl, target);
> -
> - if (!target.isPermanent()) {
> - incrementKeepAliveCount();
> + if (!target.isPermanent()) {
> + incrementKeepAliveCount();
> + }
> }
> }
> }
