neo-ConfuserEx icon indicating copy to clipboard operation
neo-ConfuserEx copied to clipboard

TypeScrambler fails when scrambling generic class

Open mkaring opened this issue 7 years ago • 12 comments

Describe the bug The type scrambler causes and error together with the renaming when handling methods in a generic class, because it does not handle the generic parameters of the class itself properly.

To Reproduce Steps to reproduce the behavior:

  1. Have an assembly that contains a class like this: https://github.com/mkaring/ConfuserEx/blob/feature/typescrambler_unittest/Tests/TypeScrambler/GenericClass.cs
  2. Try to have the type scrambler process it. Be sure that the Confuser.Rename assembly is loaded and active as well, because it's AnalyzePhase raises the error.

Expected behavior Should work without errors.

Additional context My best guess is that the scrambler does not consider the generic parameter of the class at this point. The result of the UnitTest, including the error is here: https://ci.appveyor.com/project/mkaring/confuserex/build/13#L289

mkaring avatar Jul 22 '18 11:07 mkaring

Error is raised by Confuser.Protections.TypeScramble.Scrambler not Confuser.Renamer ?

TypeSig.ScopeType is null and because there is no exception handling here it throw an error

XenocodeRCE avatar Jul 22 '18 12:07 XenocodeRCE

Ah yes. As an attempted work around I added a null Handling there and had the method return null. And that resulted in the error I described. I forgot about that, sorry.

Either way it should work. :wink:

mkaring avatar Jul 22 '18 12:07 mkaring

I don't think TypeScrambler can handle generics, because it transforms param and so to generic. I used a built-int Boolean method to check if Type contains generic types, this seems to work for the moment.

XenocodeRCE avatar Jul 22 '18 12:07 XenocodeRCE

I'll run it against my test cases and get back to you.

mkaring avatar Jul 22 '18 15:07 mkaring

From my tests it seems is simply not compatible with generic types at all.

class Test<T>
    {
        T _value;

        public Test(T t)
        {
            // The field has the same type as the parameter.
            this._value = t;
        }

        public void Write()
        {
            Console.WriteLine(this._value);
        }
    }


    class Program
    {
        static void Main(string[] args)
        {

            Test<int> test1 = new Test<int>(5);
            // Call the Write method.
            test1.Write();

            // Use the generic type Test with a string type parameter.
            Test<string> test2 = new Test<string>("cat");
			
            test2.Write();

            Console.ReadKey();
        }
    }

will inevitably fail and throw an error when TypeScrambler is selected. It seems to be a very sloppy project. I might remove it and prefer to code my own from scratch.

XenocodeRCE avatar Jul 22 '18 16:07 XenocodeRCE

Where did you get this thing from? I only got it in my repository from yours.

mkaring avatar Jul 22 '18 17:07 mkaring

@mkaring he got it from here: https://github.com/BahNahNah/ConfuserExPlugins

alexmurari avatar Jul 22 '18 18:07 alexmurari

The problem is still present. The issue occurs because that ConvertToGenericIfAvalible method returns the original type signature in case in case the scrambling of the type should fail. Now the methods calling this, get the original type signature that also contains a generic type. So they assume the method is being scrambled and at this point it all fails.

I think the basic method on how this works is the problem here. The analyzing part of the scrambler needs to annotate the methods that are selected for scrambling and the rewriter phase needs to update them based on the the annotations.

Also it is absolutely required to rename the Prepair functions to Prepare.

All in all I think most of the scrambler is usable. Just the part how the phases communicate needs to be improved to some extend.

mkaring avatar Jul 22 '18 19:07 mkaring

Last commit should have fixed this bug @mkaring thanks to 2 PR

XenocodeRCE avatar Jul 27 '18 15:07 XenocodeRCE

That is still not it. It still messes up the analysis stage of the renamer, because the method references don't match anymore.

See: https://ci.appveyor.com/project/mkaring/confuserex/build/22#L384

mkaring avatar Jul 27 '18 21:07 mkaring

Bit late but ive started a rewrite to make it a bit more readable. https://github.com/BahNahNah/ConfuserExPlugins/tree/rewrite

For whatever reason its corrupting the assembly and debugging it isnt giving me much help as to why. Removing this line stops it crashing so its from applying the generics incorrectly. https://github.com/BahNahNah/ConfuserExPlugins/blob/rewrite/TypeScramble/ScramblePhase.cs#L24

Its late so its probably just a stupid mistake somewhere, ill try and fix it when i get time.

Edit: fixed. Working . And I've added some more stuff. On the main branch now. https://github.com/BahNahNah/ConfuserExPlugins/

pigeonhands avatar Oct 21 '18 13:10 pigeonhands

People, we might just need a Pull Request here no ? Otherwise we just might put that protection in the bin ?

XenocodeRCE avatar Feb 26 '19 12:02 XenocodeRCE