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

[Alan Bateman]

Neil - I think I offered to help get this one in and I'm just wondering 
if your patch of 3/24 is the latest? Just checking as it would be good 
to get this one in before jdk7 gets locked down.

-Alan

Peter Jones wrote:
> 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.
>