libjdwp crash while debugging application ran on OpenJDK 16

[Chris Plummer]

On 6/23/21 12:21 PM, Roman Kennke wrote:
> Hi Simon,
>
>>> my colleagues recently ran into a crash in libjdwp, when measuring
>>> performance with OpenJDK 17 (early access build). The same crash was
>>> observed with OpenJDK 16.0.1, but not with OpenJDK 15.
>>>
>>> We are hoping to get a fix for the crash, before the official 
>>> OpenJDK 17
>>> release. We've opened a RHEL bugzilla ticket for this (
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1972529), but we expect 
>>> this
>>> won't be enough to resolve the crash before the release.
>>>
>
>> I wonder if this could be caused by calling commonRef_unpin on a ref 
>> that is not "pinned"? Specifically, look at the updated weakenNode:
>>
>> static jweak
>> weakenNode(JNIEnv *env, RefNode *node)
>> {
>>      if (node->strongCount == 1) {
>> ...
>>          return weakRef;
>>      } else {
>>          node->strongCount--;
>>          return node->ref;
>>      }
>> }
>>
>> if strongCount is 0, this will underflow and then delete node will 
>> take the wrong path:
>>
>> if (node->strongCount != 0) {
>>    JNI_FUNC_PTR(env,DeleteGlobalRef)(env, node->ref);
>> } else {
>>    JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, node->ref);
>> }
>>
>> The previous version of weakenNode looked like this:
>>
>> static jweak
>> weakenNode(JNIEnv *env, RefNode *node)
>> {
>>      if (node->isStrong) {
>> ...
>>          return weakRef;
>>      } else {
>>          return node->ref;
>>      }
>> }
>>
>> so a unbalanced unpin call would previously not fail in the same way.
>
> Thank, Stefan, this seems like a plausible cause for the problem. It 
> seems to me that unbalanced pin and unpin can only be caused by 
> unbalanced DisableCollection/EnableCollection in the wire protocol, 
> and I guess we don't really have much control over this and should be 
> defensive.
>
> Simon, can you test if this patch helps:
>
> http://cr.openjdk.java.net/~rkennke/fixlibjdwp.patch
>
> It basically protects from the underflow that Stefan found.
>
> It would be good to have a test case for this. Is there a way to 
> construct tests that exercise JDWP commands?
>
> Thanks,
> Roman
>
https://bugs.openjdk.java.net/browse/JDK-8269232 has been filed. There 
is more discussion there, including some caveats about the proposed fix. 
I also suggest first adding an assert just to make sure you are indeed 
triggering it. Eclipse should also look into why it likely has 
unbalanced enable/disableCollection calls.

Chris