On Apr 8, 2020, at 3:32 AM, Per Liden <per.liden@oracle.com> wrote: On 4/8/20 2:25 AM, Kim Barrett wrote:

...

The new function uses a native method whose implementation is in the VM so it can use the Access API. It is the intent that this function will be intrinsified by optimizing compilers like C2 or graal, but that hasn't been implemented yet. Bear that in mind before rushing off to change existing uses of Reference.get.

Do we have an enhancement filed for this?

Not yet.

We should make it very clear that such an intrinsic is not allowed to safepoint between loading the referent and comparing it.

...

CR: https://bugs.openjdk.java.net/browse/JDK-8188055 https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR) Webrev: https://cr.openjdk.java.net/~kbarrett/8188055/open.04/

The patch looks good to me, just two minor comments:

java/lang/ref/Reference.java ----------------------------

349 * @param obj is the object to compare with the referenced object, or 350 * {@code null}.

May I suggest "referenced object" -> "referent" to keep the nomenclature intact.

That seems reasonable, but I’m going to wait for some of the other discussion to resolve before making any changes to the Javadoc.

gc/TestReferenceRefersTo.java -----------------------------

208 long timeout = 1000; 209 while (true) { 210 Reference<? extends TestObject> ref = queue.remove(timeout); 211 if (ref == null) { 212 break; 213 } else if (ref == testPhantom1) { 214 testPhantom1 = null; 215 } else if (ref == testWeak2) { 216 testWeak2 = null; 217 } else if (ref == testWeak3) { 218 testWeak3 = null; 219 } else if (ref == testWeak4) { 220 testWeak4 = null; 221 } else { 222 fail("unexpected reference in queue"); 223 } 224 }

That timeout look unnecessarily short, and I imagine it could cause intermittent failures on really slow machines. I would suggest we make that more like 60 seconds, alternatively skip the timeout all together and let the whole test timeout if the expected references don't appear in the queue.

Increasing the timeout seems reasonable. I’m not so sure about removing the limit completely though. I think we don’t always get much state information on a test timeout, which would make it harder to distinguish this specific failure from (say) a *really* slow machine taking a long time to execute the test.

On Apr 8, 2020, at 4:11 AM, Thomas Schatzl <thomas.schatzl@oracle.com> wrote:

Hi,

On 08.04.20 02:25, Kim Barrett wrote:

...

[Note review on both core-libs and hotspot-gc-dev lists; try not to lose either when replying.] Please review a new function: java.lang.ref.Reference.refersTo. [...] CR: https://bugs.openjdk.java.net/browse/JDK-8188055 https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR) Webrev: https://cr.openjdk.java.net/~kbarrett/8188055/open.04/ Testing: mach5 tier1 Locally (linux-x64) verified the new test passes with various garbage collectors.

Lgtm.

Thomas

Thanks.

On Apr 8, 2020, at 5:27 AM, David Holmes <david.holmes@oracle.com> wrote:

Hi Kim,

Apparently I lost track of these comments and forgot to respond. I still won't be sending out a new webrev until some of the other discussion gets settled. There's been some internal discussion, but I'm currently waiting on some other folks to have time to chime in. Replies to your comments inline below.

On 8/04/2020 10:25 am, Kim Barrett wrote:

...

[Note review on both core-libs and hotspot-gc-dev lists; try not to lose either when replying.] Please review a new function: java.lang.ref.Reference.refersTo. This function is needed to test the referent of a Reference object without artificially extending the lifetime of the referent object, as may happen when calling Reference.get. Some garbage collectors require extending the lifetime of a weak referent when accessed, in order to maintain collector invariants. Lifetime extension may occur with any collector when the Reference is a SoftReference, as calling get indicates recent access. This new function also allows testing the referent of a PhantomReference, which can't be accessed by calling get.

This causes a slight conflict with the documentation for get() which states:

"Because the referent of a phantom reference is always inaccessible ..."

when the new method obviously accesses it.

I take that "inaccessible" to mean "inaccessible to the application", and refersTo doesn't make the referent accessible to the application.

...

The new function uses a native method whose implementation is in the VM so it can use the Access API. It is the intent that this function will be intrinsified by optimizing compilers like C2 or graal, but that hasn't been implemented yet. Bear that in mind before rushing off to change existing uses of Reference.get.

I assume such intrinsification is intended for JDK 15 as well though, as end users may well rush to change their code!

Looks like we missed the JDK 15 train.

...

CR: https://bugs.openjdk.java.net/browse/JDK-8188055 https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR)

In the specification:

* @param obj is the object to compare with the referenced object, or * {@code null}.

First delete "is", the commone style for @param is just to say "the xxx" or "a yyy”.

Done locally.

Second I suggest:

s/the referenced object/this reference object's referent/

Done locally.

In the apinote:

* collection cycle. {@link #refersTo(Object) refersTo} can be used to

I suggest:

* collection cycle. The {@link #refersTo(Object) refersTo} method can be used to

so we don't start a sentence with a lower-case code-font word.

Done locally.

Also a query in the apinote:

* This method returns a strong reference to the referent. This may cause * the garbage collector to treat it as strongly reachable until some later

Surely if the method returns a strong reference then the GC _will_ treat it as strongly reachable, not "may” ?

Something like refersTo is needed because an access using get may force some phases of some collectors to treat the referent as strongly reachable for some additional period, even if the application immediately drops all references to it. Other collectors may not need to do anything of the sort. And even collectors that do sometimes need to do so may not always need to do so. It's all vaguely weasel-worded because there is so much potential variation.

...

Webrev: https://cr.openjdk.java.net/~kbarrett/8188055/open.04/

Code changes look good. No comment on the test - I'll leave it to others to analyse that.

Thanks.

Lifting out of response from the JIRA issue: I always worry when proposing a change to an existing invariant, and PhantomReference currently carries the stated and specified behavior of "the referent of a phantom reference is always inaccessible". I can imagine quite a few forms of gaining new information I do not otherwise have access to by using PhantomReference::RefersTo if it allowed me to examine the current referent of a phantom reference and test to see if it is (a) null or (b) a specific object I have a reference to. Both of those would provide me with information that is impossible for me to get according to current specifications. With that newly available information one can come up with all sorts of nice things to do... Think in terms of "side-channel" as an example of the sort of thinking black hats can apply to this new knowledge, but the potential attacks are not limited to side-channels. While it will be "obviously safe" to have Reference:RefersTo(obj) provide the same information that (Reference.get() == obj) would, providing more information than that would be a change to the specified behavior of Reference types, which we should be extra paranoid about. Since PhantomReference::get returns null by definition, we should remain consistent with that in PhantomReference::refersTo

On Apr 8, 2020, at 7:56 AM, Erik Österlund <erik.osterlund@oracle.com> wrote:

Hi Gil,

Lifting out my reply to you from the JIRA issue:

In terms of breaking existing logic, I am not worried. This is a new API, that nobody is using yet. People that write new code that uses it, will have to pay attention that they are doing the right thing. We are still not exposing the phantom referent with this change. In terms of security, you can only use this API to figure out what the referent is, if you already have access to it. So that isn't really helpful for building exploits. What it could do is allow you to check which one of N objects that you already have access to is the one referred to from the PhantomReference. But in terms of security, you could also have just guessed that without this API, as you already have full access to the objects. Sounds like a classic case of "I have an exploit. Given a compromised system... X". Or have I missed something?

Thanks, /Erik

On 2020-04-08 16:25, Gil Tene wrote:

...

A very welcome change overall. However, I have concerns about the semantic change to the PhantomReference specification. I propose that PhantomReference semantics remain unchanged, and that PhantomReference:RefersTo should return true only for null.

See more in comment at https://bugs.openjdk.java.net/browse/JDK-8188055?focusedCommentId=14329319&p...

...

On Apr 7, 2020, at 5:25 PM, Kim Barrett <kim.barrett@oracle.com> wrote:

[Note review on both core-libs and hotspot-gc-dev lists; try not to lose either when replying.]

Please review a new function: java.lang.ref.Reference.refersTo.

This function is needed to test the referent of a Reference object without artificially extending the lifetime of the referent object, as may happen when calling Reference.get. Some garbage collectors require extending the lifetime of a weak referent when accessed, in order to maintain collector invariants. Lifetime extension may occur with any collector when the Reference is a SoftReference, as calling get indicates recent access. This new function also allows testing the referent of a PhantomReference, which can't be accessed by calling get.

The new function uses a native method whose implementation is in the VM so it can use the Access API. It is the intent that this function will be intrinsified by optimizing compilers like C2 or graal, but that hasn't been implemented yet. Bear that in mind before rushing off to change existing uses of Reference.get.

CR: https://bugs.openjdk.java.net/browse/JDK-8188055 https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR)

Webrev: https://cr.openjdk.java.net/~kbarrett/8188055/open.04/

Testing: mach5 tier1

Locally (linux-x64) verified the new test passes with various garbage collectors.

Erik, The fact that you have access to the objects involved (and to their contents) does not mean you already have access to the new information revealed by being able to check if a phantom reference refers to some specific object. Knowing "who uses what thing" is a lot more than just knowing "who exists" and "what are the things that exist"… Many security things leverage the fundamental difference between those sets of knowledge, and that's why we e.g. fear the effect that the emergence of quantum will likely have on on existing TLS ciphers... I could probably come up with a "reasonable code" example that would make security or correctness assumptions based on the currently specified opaqueness of phantom references, and which one would then be able to write an exploit against if we change the specified behavior . E.g. it would currently be reasonable to make use of phantom references across APIs as forms of a weak and opaque identity handles (and opposed to WeakReference which would be a weak but non-opaque handle), and build security or correctness assumptions based on that presumed opaqueness. We could go through an exercise of actually building one of these to prove a point. But we don't need an actual example exploit or a proof that the change can lead to security or correctness issues. We just need enough of a worry that such issues can arise due to the change. What I'm pointing to is the worry, and suggesting that the change in semantics is not necessary. I could phrase the issue in reverse: "What are examples where being able to determine if a phantom reference refers to a specific object is useful?" I have a feeling that at least some of those examples would also provide us with the exploit examples you ask for ;-) — Gil.

On Apr 8, 2020, at 9:13 AM, Erik Österlund <erik.osterlund@oracle.com> wrote:

Hi Gil,

Do you have an example exploit, or at least the gist of it? As I already said, any information exposed could have been just guessed (replace refersTo with random() and brute force). So if you can create an exploit based on the answer of refersTo, then your system is secure by chance. In other words, it is already compromised. Or have I missed something?

Thanks, /Erik

...

On 8 Apr 2020, at 18:05, Gil Tene <gil@azul.com> wrote:

Lifting out of response from the JIRA issue:

I always worry when proposing a change to an existing invariant, and PhantomReference currently carries the stated and specified behavior of "the referent of a phantom reference is always inaccessible".

I can imagine quite a few forms of gaining new information I do not otherwise have access to by using PhantomReference::RefersTo if it allowed me to examine the current referent of a phantom reference and test to see if it is (a) null or (b) a specific object I have a reference to. Both of those would provide me with information that is impossible for me to get according to current specifications. With that newly available information one can come up with all sorts of nice things to do... Think in terms of "side-channel" as an example of the sort of thinking black hats can apply to this new knowledge, but the potential attacks are not limited to side-channels.

While it will be "obviously safe" to have Reference:RefersTo(obj) provide the same information that (Reference.get() == obj) would, providing more information than that would be a change to the specified behavior of Reference types, which we should be extra paranoid about. Since PhantomReference::get returns null by definition, we should remain consistent with that in PhantomReference::refersTo

...

On Apr 8, 2020, at 7:56 AM, Erik Österlund <erik.osterlund@oracle.com> wrote:

Hi Gil,

Lifting out my reply to you from the JIRA issue:

In terms of breaking existing logic, I am not worried. This is a new API, that nobody is using yet. People that write new code that uses it, will have to pay attention that they are doing the right thing. We are still not exposing the phantom referent with this change. In terms of security, you can only use this API to figure out what the referent is, if you already have access to it. So that isn't really helpful for building exploits. What it could do is allow you to check which one of N objects that you already have access to is the one referred to from the PhantomReference. But in terms of security, you could also have just guessed that without this API, as you already have full access to the objects. Sounds like a classic case of "I have an exploit. Given a compromised system... X". Or have I missed something?

Thanks, /Erik

...

On 2020-04-08 16:25, Gil Tene wrote: A very welcome change overall. However, I have concerns about the semantic change to the PhantomReference specification. I propose that PhantomReference semantics remain unchanged, and that PhantomReference:RefersTo should return true only for null.

See more in comment at https://bugs.openjdk.java.net/browse/JDK-8188055?focusedCommentId=14329319&p...

...

On Apr 7, 2020, at 5:25 PM, Kim Barrett <kim.barrett@oracle.com> wrote:

[Note review on both core-libs and hotspot-gc-dev lists; try not to lose either when replying.]

Please review a new function: java.lang.ref.Reference.refersTo.

This function is needed to test the referent of a Reference object without artificially extending the lifetime of the referent object, as may happen when calling Reference.get. Some garbage collectors require extending the lifetime of a weak referent when accessed, in order to maintain collector invariants. Lifetime extension may occur with any collector when the Reference is a SoftReference, as calling get indicates recent access. This new function also allows testing the referent of a PhantomReference, which can't be accessed by calling get.

The new function uses a native method whose implementation is in the VM so it can use the Access API. It is the intent that this function will be intrinsified by optimizing compilers like C2 or graal, but that hasn't been implemented yet. Bear that in mind before rushing off to change existing uses of Reference.get.

CR: https://bugs.openjdk.java.net/browse/JDK-8188055 https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR)

Webrev: https://cr.openjdk.java.net/~kbarrett/8188055/open.04/

Testing: mach5 tier1

Locally (linux-x64) verified the new test passes with various garbage collectors.

On Apr 8, 2020, at 12:19 PM, Mandy Chung <mandy.chung@oracle.com> wrote:

I agree with Gil on this point. `PhantomReference::get` always returns null. The intent behavior of `ref.refersTo(obj)` is the same as `ref.get() == obj`. Gil's proposed option to have `refersTo(obj)` to return true only if obj is null is a reasonable one.

If `PhantomReference::get` were to change to have the same behavior as other references some day, then `PhantomReference::refersTo` would be updated at that time (but no proposal doing that at the moment).

Mandy

I disagree. Prior to JDK-8071507, PhantomReference.get was needed to prevent access to the possibly reclaimable referent. That's even mentioned in the PhantomReference class documentation (the final paragraph; maybe that should have been modified as part of JDK-8071507). Not that PhantomReference.get has ever been an air-tight wall, as things like Unsafe, reflection, and perhaps others (JVMTI?) might still provide access or information (JDK-8168804). But after JDK-8071507 there's at least no longer any risk of those avenues providing access to a referent after the PhantomReference has been notified and any associated cleanup triggered by that notification has been performed. I think we should remove PhantomReference.get, and have been discussing this with Mandy. (Note that there are some technical details that make that more difficult to implement than simply removing the method; C2 is a pain.) That method prevents the use of PhantomReference in contexts that would benefit from its different liveness behavior from WeakReference with respect to finalization. For example, consider a PhantomReference variant of the existing java.util.WeakHashMap. Then, in a world without finalization (I should live so long), we could collapse the hierarchy down because there would no longer be any difference between WeakReference and PhantomReference. Even before any future removal of PhantomReference.get I think refersTo should not be tied to Reference.get behavior. It's intended that refersTo() not call get() (and that probably ought to be stated explicitly, similarly to how Reference.clear is explicitly not called by the GC to clear references). If it calls get then it affects SoftReferences as well, again potentially extending the lifetimes of objects unrelated to the one in hand.

On 8/25/20 10:11 PM, Kim Barrett wrote:

Finally getting back to this after some internal discussion that dragged along because folks have been busy.

...

On Apr 8, 2020, at 12:05 PM, Gil Tene <gil@azul.com> wrote:

Lifting out of response from the JIRA issue:

I always worry when proposing a change to an existing invariant, and PhantomReference currently carries the stated and specified behavior of "the referent of a phantom reference is always inaccessible".

I can imagine quite a few forms of gaining new information I do not otherwise have access to by using PhantomReference::RefersTo if it allowed me to examine the current referent of a phantom reference and test to see if it is (a) null or (b) a specific object I have a reference to. Both of those would provide me with information that is impossible for me to get according to current specifications. With that newly available information one can come up with all sorts of nice things to do... Think in terms of "side-channel" as an example of the sort of thinking black hats can apply to this new knowledge, but the potential attacks are not limited to side-channels.

While it will be "obviously safe" to have Reference:RefersTo(obj) provide the same information that (Reference.get() == obj) would, providing more information than that would be a change to the specified behavior of Reference types, which we should be extra paranoid about. Since PhantomReference::get returns null by definition, we should remain consistent with that in PhantomReference::refersTo [copied from JIRA issue] That all assumes that I'm giving you access to my phantom references. But if I'm doing that then I've got much worse problems than any potential information leakage of the kind being suggested. If you have one of my phantom references then you can enqueue it, triggering any associated cleanup while I'm still using the referent. One should not give one's phantom references to untrusted code. That's nothing new, and is not made any worse by having Reference.refersTo work the same way for phantom references as for other reference types.

Agree.    We should probably document this in the Java security code guideline to advice the developers be cautious and not to leak Reference objects. Mandy

On Apr 9, 2020, at 11:31 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

Reference.java —

335 * 336 * @return The object to which this reference refers, or 337 * {@code null} if this reference object has been cleared

Add @see refersTo

338 */ 339 @HotSpotIntrinsicCandidate 340 public T get() {

Thanks. Changed locally; waiting for other discussions to converge before posting any new webrevs.