Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScrambler fails when scrambling generic class #5

Open
mkaring opened this issue Jul 22, 2018 · 12 comments
Open

TypeScrambler fails when scrambling generic class #5

mkaring opened this issue Jul 22, 2018 · 12 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@mkaring
Copy link

mkaring commented Jul 22, 2018

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

@XenocodeRCE XenocodeRCE added the bug Something isn't working label Jul 22, 2018
@XenocodeRCE
Copy link
Owner

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

@mkaring
Copy link
Author

mkaring commented Jul 22, 2018

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. 😉

XenocodeRCE added a commit that referenced this issue Jul 22, 2018
@XenocodeRCE
Copy link
Owner

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.

@mkaring
Copy link
Author

mkaring commented Jul 22, 2018

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

@XenocodeRCE
Copy link
Owner

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.

@mkaring
Copy link
Author

mkaring commented Jul 22, 2018

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

@alexmurari
Copy link

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

@mkaring
Copy link
Author

mkaring commented Jul 22, 2018

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.

@XenocodeRCE
Copy link
Owner

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

@mkaring
Copy link
Author

mkaring commented Jul 27, 2018

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

@XenocodeRCE XenocodeRCE added the help wanted Extra attention is needed label Aug 10, 2018
@pigeonhands
Copy link

pigeonhands commented Oct 21, 2018

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/

@XenocodeRCE
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants