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

It's not safe to Load InitType with type name only #382

Open
rugbbyli opened this issue May 12, 2020 · 2 comments
Open

It's not safe to Load InitType with type name only #382

rugbbyli opened this issue May 12, 2020 · 2 comments

Comments

@rugbbyli
Copy link

Sorry, this issue is for ClojureCLR (https://github.com/arcadia-unity/clojure-clr), but that project does not accept issue, so i write it here.

When ClojureCLR invoke a clojure code, it iterate over all assemblies and find the type with initClassName matched. source code here:

        internal static bool TryLoadInitType(string relativePath)
        {
            var initClassName = InitClassName(relativePath);
            Type initType = null;
            foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())
            {
#if CLR2
                if(asm.ManifestModule is ModuleBuilder)
#else
                if (asm.IsDynamic)
#endif
                    continue;
                initType = asm.GetType(initClassName);
                if (initType != null)
                    break;
            }
            if (initType == null)
                return false;

            InvokeInitType(initType.Assembly, initType);
            return true;
        }

But it's not safe, as there may be another type have a same name , and would be wrongly loaded.

To prevent this situation, we could double check using excepted assembly name, like this:

        internal static bool TryLoadInitType(string relativePath)
        {
            var initClassName = InitClassName(relativePath);
            Type initType = null;
            var asmName = relativePath.Replace('/', '.') + ".clj";
            foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())
            {
#if CLR2
                if(asm.ManifestModule is ModuleBuilder)
#else
                if (asm.IsDynamic)
#endif
                    continue;
                if(!asm.GetName(false).Name.StartsWith(asmName))
                    continue;
                initType = asm.GetType(initClassName);
                if (initType != null)
                    break;
            }
            if (initType == null)
                return false;

            InvokeInitType(initType.Assembly, initType);
            return true;
        }
@nasser
Copy link
Contributor

nasser commented May 24, 2020

that's a fair point. this is not an issue in practice, because the name of the type returned by InitClassName has symbols like $ in it, so you could not actually write a type with such a name in c# (or even in clojure, because we prefix type names with the namespaces they were defined in). an additional check would not hurt anything, though. i am focused on finishing MAGIC for the next few months so i will not be able to hack on ClojureCLR myself. if you want to take out a PR against https://github.com/arcadia-unity/clojure-clr i will merge that in once you've signed the clojure contributor's agreement (required by upstream).

@rugbbyli
Copy link
Author

Yes, normally it's safe, while when building in Unity3D and enable strip code, it made an "Incompatibility" with unity3d linker, and cause a bug here.
Personally, i thought it may be a problem with unity, but that's a complex prove, and could easier bypass it using the way above.
I would check the clojure contributor agreement later and try to make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants