mgravell / fast-member

Automatically exported from code.google.com/p/fast-member
Apache License 2.0
1.03k stars 139 forks source link

No safeguard against public ref getters #75

Closed BreyerW closed 5 years ago

BreyerW commented 5 years ago

If you define public ref getter FastMember simply break application without even throwing exception. At very least IsByRef checks must be added to guard against such members.

On a side note @mgravell do you think could you normally support ref returning getters? Setter can be simulated by wrapping in Action<object,object> (for open delegate) the call to getter on the left side of assignment and passing set value on the right side of assignment. When generating getter ref return doesnt matter thanks to implict dereference.

Edit: extra note - if you think about adding ref getter support keep in mind CanWrite is false for propertyInfo with ref return but you can check IsByRef and safely assume its ref returning getter when true

mgravell commented 5 years ago

Agree that we need the check. I guess that since C# only recently supports ref-return, we simply never noticed before.

Proxying it under the existing API is very hard though - probably impossible. By design, you can't box these things. The runtime is right to hate us here! Anything we did here to support this scenario would need a different API.

BreyerW commented 5 years ago

@mgravell Actually support for ref returning getter under weakly typed delegates with existing API is possible which i managed to do so myself but i will admit it was nightmare due to lack of support in Expression tree (and without using ilgenerator since i know very little about il). 2 whole days - including good part of nights - of fighting with this only to come with very hackish solution that threw Expression tree out of the window - shame but couldnt be helped (basically few indirect calls to factory methods through reflection - can be cached of course)

Here is unclean, uncached code (very fresh, did it today)

using System;
using System.Linq.Expressions;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.CompilerServices;

namespace Sharp
{
    public delegate void RefAction<T1, T2>(T1 instance, ref T2 value);
    internal delegate ref TResult RefFunc<T, TResult>(T instance);
    internal static class DelegateGenerator
    {
        /// <summary>
        /// Generate open setter for field or property
        /// </summary>
        /// <typeparam name="T">Type of passed value that will be assigned to field/property</typeparam>
        /// <param name="memberInfo">Field or property info</param>
        /// <returns></returns>
        public static Action<object, T> GenerateSetter<T>(MemberInfo memberInfo)
        {
            if (memberInfo.GetUnderlyingType() is { IsByRef: true })
            {
                var generator = typeof(DelegateGenerator).GetMethod(nameof(RefGetterAsSetterHelper), BindingFlags.NonPublic | BindingFlags.Static).MakeGenericMethod(typeof(T), memberInfo.GetUnderlyingType().GetElementType(), memberInfo.DeclaringType);
                var setter = (Action<object, T>)generator.Invoke(null, new[] { memberInfo });
                return setter;
            }
            }
/// <summary>
        /// Generate open getter for field or property
        /// </summary>
        /// <typeparam name="T">Type of returned value from field/property getter</typeparam>
        /// <param name="memberInfo">Field or property info</param>
        /// <returns></returns>
        public static Func<object, T> GenerateGetter<T>(MemberInfo memberInfo)
        {
            if (memberInfo.GetUnderlyingType() is { IsByRef: true })
            {
                var generator = typeof(DelegateGenerator).GetMethod(nameof(RefGetterHelper), BindingFlags.NonPublic | BindingFlags.Static).MakeGenericMethod(typeof(T), memberInfo.GetUnderlyingType().GetElementType(), memberInfo.DeclaringType);
                return (Func<object, T>)generator.Invoke(null, new[] { memberInfo });
            }
        }
        private static Action<object, TRequested> RefGetterAsSetterHelper<TRequested, TReal, TInstance>(MemberInfo memberInfo)
        {
            var getter = CreateRefGetter<TReal>(memberInfo) as RefFunc<TInstance, TReal>;
            return (object instance, TRequested val) => { getter((TInstance)instance) = (TReal)(object)val; };
        }
        private static Func<object, TRequested> RefGetterHelper<TRequested, TReal, TInstance>(MemberInfo memberInfo)
        {
            var getter = CreateRefGetter<TReal>(memberInfo) as RefFunc<TInstance, TReal>;
            return (instance) => (TRequested)(object)getter((TInstance)instance);
        }
        private static Delegate CreateRefGetter<T>(MemberInfo memberInfo)
        {
            return Delegate.CreateDelegate(typeof(RefFunc<,>).MakeGenericType(memberInfo.DeclaringType, typeof(T)), (memberInfo as PropertyInfo).GetMethod);
        }

}

first two methods show how to use this hack for getter and setter generation separately all the rest is pure hack just to make first two working (note i deleted a lot of code unrelated to ref getter in order to trim down the wall of text so if you try to compile as-is it wont work).

TRequested is a type that you want to have in public signature AND is compatible with member type (eg if you want weakly typed delegate you call GenerateSetter<object> and you get Action<object,object> back).

TReal is real type of member (field/property) - if you have ref returning Position property backed by Vector3but want weakly typed delegate then TRequested must be object and TReal must be Vector3

TInstance is a real type of class from which we call ref property.

BreyerW commented 5 years ago

@mgravell sorry for double post but i think im justified since i managed to convert this hack to nice IL emitted code thanks to csharplab. I battle tested this in my pet project where i have ref returning getters sprinkled all over. Here it is:


public static Action<object, T> GenerateSetter<T>(MemberInfo memberInfo)
{
            if (memberInfo.GetUnderlyingType() is { IsByRef: true })
            {
                var method = new DynamicMethod("", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, null, new[] { typeof(object), typeof(T)}, memberInfo.DeclaringType, true);
                var il = method.GetILGenerator();
                var get = (memberInfo as PropertyInfo).GetGetMethod();
                var memType = memberInfo.GetUnderlyingType().GetElementType();
                il.DeclareLocal(memberInfo.DeclaringType);
                il.Emit(OpCodes.Ldarg_0);
                il.Emit(OpCodes.Castclass, memberInfo.DeclaringType);
                il.Emit(OpCodes.Callvirt, get);
                il.Emit(OpCodes.Ldarg_1);
                if (memType.IsClass)
                {
                    il.Emit(OpCodes.Castclass, memType);
                    il.Emit(OpCodes.Stind_Ref);
                }
                else
                {
                    il.Emit(OpCodes.Unbox_Any, memType);
                    il.Emit(OpCodes.Stobj, memType);
                }
                il.Emit(OpCodes.Ret);
                return method.CreateDelegate(typeof(Action<object, T>)) as Action<object, T>;
                         }
}
public static Func<object, T> GenerateGetter<T>(MemberInfo memberInfo)
        {
            if (memberInfo.GetUnderlyingType() is { IsByRef: true })
            {
                var method = new DynamicMethod("", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(T), new[] { typeof(object) }, memberInfo.DeclaringType, true);
                var il = method.GetILGenerator();
                var get = (memberInfo as PropertyInfo).GetGetMethod();
                var type = memberInfo.GetUnderlyingType().GetElementType();
                il.Emit(OpCodes.Ldarg_0);
                il.Emit(OpCodes.Castclass, memberInfo.DeclaringType);
                il.Emit(OpCodes.Callvirt, get);
                if (type.IsClass)
                {
                    il.Emit(OpCodes.Ldind_Ref);
                }
                else
                {
                    il.Emit(OpCodes.Ldobj, type);
                    il.Emit(OpCodes.Box, type);
                }
                                il.Emit(OpCodes.Unbox_Any, typeof(T));//just in case there is type mismatch between member type and T. Can be removed if you care only about weakly typed delegates
                il.Emit(OpCodes.Ret);
                return method.CreateDelegate(typeof(Func<object, T>)) as Func<object, T>;
                         }
}

I think it goes without saying this also work for Action<object,object> (setter) and Func<object,object> (getter) but if you want to be more strongly typed you can (only on set value in setter and return in getter though). Both are unbounded (open) delegates.

This is only for properties and this work only with classes at which we call property (property itself can be a struct or class) but in this case we dont care since field cannot be ref anyway.

Adding support for calling ref returning property on struct is almost useless since struct cant return ref to itself (or its fields). There could be rare case where ref returning getter on struct grabs data outside of itself but i think thats so rare it will be fine not to support and in future - if necessary - csharplab can show how to implement support for such case (with some legwork).

Plus extension class i made for MemberInfo and is used in this code snippet so i include it.


internal static class DelegateGeneratorEx
    {
        public static Type GetUnderlyingType(this MemberInfo member)
        {
            switch (member.MemberType)
            {
                case MemberTypes.Event:
                    return ((EventInfo)member).EventHandlerType;

                case MemberTypes.Field:
                    return ((FieldInfo)member).FieldType;

                case MemberTypes.Method:
                    return ((MethodInfo)member).ReturnType;

                case MemberTypes.Property:
                    return ((PropertyInfo)member).PropertyType;

                default:
                    throw new ArgumentException
                    (
                     "Input MemberInfo must be if type EventInfo, FieldInfo, MethodInfo, or PropertyInfo"
                    );
            }
        }
    }

this is code snippet i used to "feed" csharplab

//for setter

using System;

public class Program
{
    public class Mesh{}
    public static void Main()
    {
        Console.WriteLine("Hello World");
    }
    public class MeshRenderer
    {
        private Mesh mesh;
        public ref Mesh Mesh(){
            return ref mesh;
        }
        public static void test(object instance, object val){
        var casted=(MeshRenderer)instance;
            casted.Mesh()=(Mesh)val;
        }
    }
}

//for getter

using System;

public class Program
{
    public class Mesh{}
    public static void Main()
    {
        Console.WriteLine("Hello World");
    }
    public class MeshRenderer
    {
        private Mesh mesh;
        public ref Mesh Mesh(){
            return ref mesh;
        }
        public static object test(object instance){
                        var casted=(MeshRenderer)instance;
            return casted.Mesh();
        }
    }
}

if you want to double check my IL code, see generated IL in csharplab then simply change class Mesh to struct Mesh, see IL again and you covered both class and struct. I used Release mode to get the shortest working code.

mgravell commented 5 years ago

Oh, you're talking about dereferencing the pointer in the IL? Yes, that'll work, but it kinda defeats the usual purpose of having a ref-return...

Better than nothing, perhaps, though. Interesting.

On Fri, 24 May 2019, 01:45 BreyerW, notifications@github.com wrote:

@mgravell https://github.com/mgravell sorry for double post but i think im justified since i managed to convert this hack to nice IL emitted code thanks to csharplab. I battle tested this in my pet project where i have ref returning getters sprinkled all over. Here it is:

public static Action<object, T> GenerateSetter(MemberInfo memberInfo) { if (memberInfo.GetUnderlyingType() is { IsByRef: true }) { var method = new DynamicMethod("", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, null, new[] { typeof(object), typeof(T)}, memberInfo.DeclaringType, true); var il = method.GetILGenerator(); var get = (memberInfo as PropertyInfo).GetGetMethod(); var memType = memberInfo.GetUnderlyingType().GetElementType(); il.DeclareLocal(memberInfo.DeclaringType); il.Emit(OpCodes.Ldarg_0); il.Emit(OpCodes.Castclass, memberInfo.DeclaringType); il.Emit(OpCodes.Callvirt, get); il.Emit(OpCodes.Ldarg_1); if (memType.IsClass) { il.Emit(OpCodes.Castclass, memType); il.Emit(OpCodes.Stind_Ref); } else { il.Emit(OpCodes.Unbox_Any, memType); il.Emit(OpCodes.Stobj, memType); } il.Emit(OpCodes.Ret); return method.CreateDelegate(typeof(Action<object, T>)) as Action<object, T>; } }public static Func<object, T> GenerateGetter(MemberInfo memberInfo) { if (memberInfo.GetUnderlyingType() is { IsByRef: true }) { var method = new DynamicMethod("", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(T), new[] { typeof(object) }, memberInfo.DeclaringType, true); var il = method.GetILGenerator(); var get = (memberInfo as PropertyInfo).GetGetMethod(); var type = memberInfo.GetUnderlyingType().GetElementType(); il.Emit(OpCodes.Ldarg_0); il.Emit(OpCodes.Castclass, memberInfo.DeclaringType); il.Emit(OpCodes.Callvirt, get); if (type.IsClass) { il.Emit(OpCodes.Ldind_Ref); } else { il.Emit(OpCodes.Ldobj, type); il.Emit(OpCodes.Box, type); } il.Emit(OpCodes.Ret); return method.CreateDelegate(typeof(Func<object, T>)) as Func<object, T>; } }

I think it goes without saying this also work for Action<object,object> (setter) and Func<object,object> (getter) but if you want to be more strongly typed you can (only on set value in setter and return in getter though). Both are unbounded (open) delegates.

This is only for properties and this work only with classes at which we call property (property itself can be a struct or class) but in this case we dont care since field cannot be ref anyway.

Adding support for calling ref returning property on struct is almost useless since struct cant return ref to itself (or its fields). There could be rare case where ref returning getter on struct grabs data outside of itself but i think thats so rare it will be fine not to support and in future - if necessary - csharplab can show how to implement support for such case (with some legwork).

Plus extension class i made for MemberInfo and is used in this code snippet so i include it.

internal static class DelegateGeneratorEx { public static Type GetUnderlyingType(this MemberInfo member) { switch (member.MemberType) { case MemberTypes.Event: return ((EventInfo)member).EventHandlerType;

          case MemberTypes.Field:
              return ((FieldInfo)member).FieldType;

          case MemberTypes.Method:
              return ((MethodInfo)member).ReturnType;

          case MemberTypes.Property:
              return ((PropertyInfo)member).PropertyType;

          default:
              throw new ArgumentException
              (
               "Input MemberInfo must be if type EventInfo, FieldInfo, MethodInfo, or PropertyInfo"
              );
      }
  }

}

this is code snippet i used to "feed" csharplab

//for setter using System; public class Program { public class Mesh{} public static void Main() { Console.WriteLine("Hello World"); } public class MeshRenderer { private Mesh mesh; public ref Mesh Mesh(){ return ref mesh; } public static void test(object instance, object val){ var casted=(MeshRenderer)instance; casted.Mesh()=(Mesh)val; } } } //for getter using System; public class Program { public class Mesh{} public static void Main() { Console.WriteLine("Hello World"); } public class MeshRenderer { private Mesh mesh; public ref Mesh Mesh(){ return ref mesh; } public static object test(object instance){ var casted=(MeshRenderer)instance; return casted.Mesh(); } } }

if you want to double check my IL code, see generated IL in csharplab then simply change class Mesh to struct Mesh, see IL again and you covered both class and struct. I used Release mode to get the shortest working code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mgravell/fast-member/issues/75?email_source=notifications&email_token=AAAEHMBJ3GJZA4DPA6SD4UDPW43DBA5CNFSM4HOV3ZRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWD3LFI#issuecomment-495433109, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAEHMAUTSUAIWKI7HL5OHDPW43DBANCNFSM4HOV3ZRA .

BreyerW commented 5 years ago

@mgravell Keep in mind that i generate 2x delegates per ref returning getter in such a way that lack of ref return can be glossed over. fastMember[obj,"MyRefReturningProperty"]=myValToBeSet; simply work with my solution.

Getter is dereferencing because i kept in mind how it could be integrated into FastMember and thats unavoidable if we want to be weakly typed (and DynamicMethod doesnt support byref on return but support byref on parameters - i think if you reeeeeaaaly wanted ref then that fact could be abused or maybe switch to MethodBuilder but i dont know if builder support byref on return). Plus i must remind ref types are special in a way that you cant cast them to each other even if types are compatible so ref Vector3 to ref object and back is no-no (well with ref returning Unbox going back for structs is kinda possible).

But setter doesnt dereference and works ok (then again i wanted to be flexible with types so casting value that is to be set to property is necessary. If flexibility with types wasnt necessary then that could be removed completely). I tested this in undo/redo system where i set all properties directly some which are ref returning getters.

And btw internally i use delegates with ref parameter for value to be set but since FastMember doesnt use them i converted my samples to avoid it completely (difference is surprisingly small - adding MakeByRefType() in parameter list and emit ldind.ref in correct place and thats all). I didnt bothered with ref for instance object since they are all classes anyway.

mgravell commented 5 years ago

I think the best we can do with the current API is make the properties work much like they would in regular C#, i.e. if you had:

object val = accessor["SomeByRefProperty"];
// equiv to: object val = someObj.SomeByRefProperty;

and

object val = ...
accessor["SomeByRefProperty"] = val;
// equiv to: someObj.SomeByRefProperty = (ThePropType)val;

(with the latter not working for ref-readonly)

This avoids the error, and gives the intuitive - if suboptimal - results. For optimal results, you kinda need a different API.

BreyerW commented 5 years ago

@mgravell yeah agreed thats enough. Thanks for replies.

mgravell commented 5 years ago

This is in 1.5.0 btw

BreyerW commented 5 years ago

Tested it as soon as i saw it so far everything works as excpected (setting,getting). Thanks again!