Request for review: 6597112: Referential integrity loophole during remote object export

[Peter Jones]

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.richards at ngmr.net>, <neil_richards at 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.richards at 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();
> +                }
>             }
>         }
>     }