Use protected as the minimum visibilty in open classes #132

The current implementation of VisibiltyTransformer determines visibility
using the owning classes at the access sites. This is insufficient in
the unusual case where a parent class accesses one of its own members
via one of its child classes, for example:

class A {
    int x; // current implementation will mark this as private, not protected

    void fn() {
        B b = B();
        b.x = 1; // accessing a member of A via B
    }
}

class B extends A { }

An example of this is in client build 550,
client!mo.a(Ljava/awt/Component;BILsignlink!pm;I)Lclient!tj;

To correct this, the minimum visibility for members in an open class is
now protected, not private. In many cases this results in members having
a wider access than necessary. A more sophisticated approach would be to
determine the type (or possible types) used at the access site, so only
members that are accessed in this unusual manner need to have their
visibility weakened. This requires type analysis of everything on the
stack.

This change also introduces an implicit dependency from
VisibilityTransformer to FinalTransformer, as running the latter first
will minimise the set of non-final classes.

Signed-off-by: Sophia git@sophia.gg

Good catch!

This requires type analysis of everything on the stack.

Does it, or can the owner from the MemberInsnNode be used? (still thinking about this...)

Good find.

This may have been the reason for the bytecode deobfuscator failed on my attempt (Class360->Signlink (client build #667)).

This requires type analysis of everything on the stack.

Does it, or can the owner from the MemberInsnNode be used? (still thinking about this...)

The owner is the class the member is declared in, as far as I can see, so in the above example, the FieldInsnNode for the x store has an owner of A. This doesn't give any information about which type has actually been declared, so we need to know the type of the reference on the top of the stack when the MemberInsnNode is invoked.

~$ cat Test.java 
class Test {
    static class A {
        int test;
    }

    static class B extends A {
    }

    static void main(String[] args) {
        B b = new B();
        System.out.println(b.test);

        A a = b;
        System.out.println(a.test);
    }
}
~$ javac Test.java
~$ javap -c Test
Compiled from "Test.java"
class Test {
  Test();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  static void main(java.lang.String[]);
    Code:
       0: new           #2                  // class Test$B
       3: dup
       4: invokespecial #3                  // Method Test$B."<init>":()V
       7: astore_1
       8: getstatic     #4                  // Field java/lang/System.out:Ljava/io/PrintStream;
      11: aload_1
      12: getfield      #5                  // Field Test$B.test:I
      15: invokevirtual #6                  // Method java/io/PrintStream.println:(I)V
      18: aload_1
      19: astore_2
      20: getstatic     #4                  // Field java/lang/System.out:Ljava/io/PrintStream;
      23: aload_2
      24: getfield      #7                  // Field Test$A.test:I
      27: invokevirtual #6                  // Method java/io/PrintStream.println:(I)V
      30: return
}
~$

javac will set the owner to the type of the declaration - e.g. in the above example:

12: getfield #5 // Field Test$B.test:I corresponds to b.test
and
24: getfield #7 // Field Test$A.test:I corresponds to a.test

even though in both cases test is declared in A.

javac will set the owner to the type of the declaration even though in both cases test is declared in A.

The current version of javac may do this, but this is not required by the JVM specification, and either a past version of javac or the Runescape obfuscator does not seem to have followed the behaviour you're seeing. We can see an example of this in the following section of the method mentioned above (client!mo.a(Ljava/awt/Component;BILsignlink!pm;I)Lclient!tj;, currently called AudioChannel.create):

@Pc(129) SignLinkAudioChannel channelx = new SignLinkAudioChannel(signLink, channelId);
channelx.sampleRate = sampleRatex;

This FieldInsnNode has an owner of AudioChannel.

     270: invokespecial #239                // Method nm."<init>":(Lpm;I)V -- SignLinkAudioChannel
     273: astore        5
     275: aload         5
     277: iload_2
     278: putfield      #197                // Field tj.C:I -- AudioChannel.sampleRate

ah okay, if javac or the Jagex obfuscator emits it that's fine.

I do still think the type information is useful though.

The bytecode verifier ensures that the type in the GET/PUTFIELD instruction is assignable from the inferred type of the variable - so I think it's fair to make that assumption.

The deobfuscator's createInheritedFieldSets() stuff could then be used to check if any field in the set has an owner matching the containing classs?