[PATCH][asmtools] Fix incorrect handling of some CPX2 constant pool entries by JDIS

[Maxim Degtyarev]

I think in such case we should at least get rid of `switch (cns.tag)`
in `ConstantPool::ConstantStrValue()` by replacing it with `if (cns
instanceof CPX2)`
Updated patch attached.

BTW, I saw my original message was forwarded to the asmtools-dev, but
that mailing list looks dead. Which mailing list should be used for
asmtools-related issues?


2018-03-14 22:04 GMT+03:00 Leonid Kuskov <Leonid.Kuskov at oracle.com>:
> Hi Maxim,
>
> Thank you for the patch. Indeed it does fix a bug - the fix is correct but
> it seems to be overloaded with extra code.
> CX2 class contains enough information to split structures into 2 groups:
> referencing to CLASS_info
> (Fieldref, Methodref and InterfaceMethodref) and all others, hence the
> marker class CPX2_ClassMember is not necessary.
> There you can just add one method to CX2 as follows:
> boolean refersClassMember() {
>     return tag == CONSTANT_FIELD || tag == CONSTANT_METHOD || tag ==
> CONSTANT_INTERFACEMETHOD;
> }
> and change only 1 line:
> -if (cns2.value1 == cd.this_cpx) {
>
> +if ( cns2.value1 == cd.this_cpx && cns2.refersClassMember() ) {
>
> What do you think if I use this shorter fix as your contribution?
>
> Thanks,
> Leonid
>
>
> On 3/12/18 16:52, Maxim Degtyarev wrote:
>>
>> The proposed patch (see attached file jdis-cpx2-bug.diff.txt) address
>> incorrect handling of some constant pool entries represented with CPX2
>> class by ASMTOOLS JDIS.
>>
>> JDIS incorrectly assume that field CPX2::value1 is always used to hold
>> index of constant pool entry of type CONSTANT_CLASS.
>> This assumption is valid only for entries of type CONSTANT_FIELD,
>> CONSTANT_METHOD and CONSTANT_INTERFACEMETHOD.
>> For CONSTANT_NAMEANDTYPE, CONSTANT_METHODHANDLE, CONSTANT_DYNAMIC and
>> CONSTANT_INVOKEDYNAMIC this assumption is wrong.
>> As a result asmtools jdis may produce malformed output for valid input.
>>
>> Example:
>>
>> public class Test {
>>
>>      // Force `class Test` entry appear in the constant pool before
>> InvokeDynamic entries.
>>      public Class<?> CLASS = Test.class;
>>
>>      private static Runnable[] workers = {
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test"),
>>          () -> System.out.println("Test")
>>      };
>>
>>      public static void main(String... args) {
>>
>>          for(Runnable worker : workers) {
>>              worker.run();
>>          }
>>
>>      }
>>
>> }
>>
>> One of invokedynamic instructions may be rendered by jdis as
>>
>>          invokedynamic    InvokeDynamic run:"()Ljava/lang/Runnable;";
>>
>> instead of
>>
>>          invokedynamic    InvokeDynamic
>>
>> REF_invokeStatic:java/lang/invoke/LambdaMetafactory.metafactory:"(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;":run:"()Ljava/lang/Runnable;"
>> MethodType "()V", MethodHandle
>> REF_invokeStatic:Test.lambda$static$1:"()V", MethodType "()V";
>
>
-------------- next part --------------
# HG changeset patch
# User maccimo
# Date 1521257710 -10800
#      Sat Mar 17 06:35:10 2018 +0300
# Node ID 9bb02bff6c94157521bd91a8a923d4d6820b1b9f
# Parent  2904229ed97ec1d90a055ad86759f867cdc5296b
Fix incorrect handling of constant pool entries of type CONSTANT_NAMEANDTYPE, CONSTANT_METHODHANDLE, CONSTANT_DYNAMIC and CONSTANT_INVOKEDYNAMIC.

diff --git a/src/org/openjdk/asmtools/jdis/ConstantPool.java b/src/org/openjdk/asmtools/jdis/ConstantPool.java
--- a/src/org/openjdk/asmtools/jdis/ConstantPool.java
+++ b/src/org/openjdk/asmtools/jdis/ConstantPool.java
@@ -598,6 +598,10 @@
                     break;
             }
         }
+
+        public boolean refersClassMember() {
+            return tag == TAG.CONSTANT_FIELD || tag == TAG.CONSTANT_METHOD || tag == TAG.CONSTANT_INTERFACEMETHOD;
+        }
     }
 
     /* -------------------------------------------------------- */
@@ -998,17 +1002,10 @@
         if (cns == null) {
             return "#" + cpx;
         }
-        switch (cns.tag) {
-            case CONSTANT_METHODHANDLE:
-            case CONSTANT_DYNAMIC:
-            case CONSTANT_INVOKEDYNAMIC:
-            case CONSTANT_METHOD:
-            case CONSTANT_INTERFACEMETHOD:
-            case CONSTANT_FIELD: {
-                CPX2 cns2 = (CPX2) cns;
-                if (cns2.value1 == cd.this_cpx) {
-                    cpx = cns2.value2;
-                }
+        if (cns instanceof CPX2) {
+            CPX2 cns2 = (CPX2) cns;
+            if (cns2.value1 == cd.this_cpx && cns2.refersClassMember()) {
+                cpx = cns2.value2;
             }
         }
         return cns.tag.tagname + " " + StringValue(cpx);