Strict enum value checking

[ixje]

Summary
Enum's in C# are not strict when it comes to casting. e.g.

public enum VMState : byte
    {
        NONE = 0,

        HALT = 1 << 0,
        FAULT = 1 << 1,
        BREAK = 1 << 2,
    }

VMState state = (VMState) 8; <- works fine

This can lead to various issues or performance degradation;

• In NEO2 a contract was successfully deployed with an invalid ContractParameterType (I'm running neo-cli (2.7.4.0) and ApplicationLogs are not created! #228). The impact for this specific case was low but could be worse.
• Objects can pass deserialization with invalid data (example) leading to unnecessary resource consumption, where we could have thrown an exception, aborted early and save resources.
• The VM can run longer than necessary with invalid data (interop layer example that could abort early)

Do you have any solution you want to propose?
I'd like to suggest we create some helper method that does strict Enum value checking and throws an exception if it is out of bounds.

So the above could look something like

VMState state = Helper.StrictEnumFrom<VMState>(8); <- raises FormatException

Where in the software does this update applies to?
Everywhere

I'm interested to hear thoughts on the idea

[ixje]

The implementation could be something along the lines of this (not working, but for showing the idea)

    public static class EnumHelper
    {
        public static T StrictEnumFrom<T>(int value) where T : Enum
        {
            T state = (T)value;
            if (!EnumValueCache<T>.DefinedValues.Contains(state))
                throw new FormatException();
            return state;
        }

        private static class EnumValueCache<T> where T : Enum
        {
            public static readonly HashSet<T> DefinedValues = new HashSet<T>((T[])Enum.GetValues(typeof(T)));
        }
    }

Enum.IsDefined() is apparently slow (according to SO posts), so the cache should help avoid a performance hit every time we call Enum.GetValues for the type.

[shargon]

I like it, only when the enum is not a flag.

[shargon]

Sometimes we do this check

neo/neo/Network/P2P/Payloads/InvPayload.cs

Lines 39 to 41 in af85f3c

	Type = (InventoryType)reader.ReadByte();
	if (!Enum.IsDefined(typeof(InventoryType), Type))
	throw new FormatException();

[ixje]

Sometimes we do this check

neo/neo/Network/P2P/Payloads/InvPayload.cs

Lines 39 to 41 in af85f3c

	Type = (InventoryType)reader.ReadByte();
	if (!Enum.IsDefined(typeof(InventoryType), Type))
	throw new FormatException();

It is great that we at least do it sometimes, but I hope we can get to some coding rule where we say "Every non-flag enum in NEO should be strict" and then to make it easy have some helper function to take care of it.

@shargon is there any reason why we couldn't make an equivalent helper for Flag enums? I still kind of expect that an enum also does not accept any invalid flags. For example I expect this to fail if GetBigInteger() returns any value that is not 0 or 1 (=the valid possible Flags)

neo/neo/SmartContract/InteropService.cs

Line 576 in f1d6434

StorageFlags flags = (StorageFlags)(byte)engine.CurrentContext.EvaluationStack.Pop().GetBigInteger();

PS: If you happen to know a solution for T state = (T)value; not compiling let me know. T state = (T)(object)value; allows it to compile, but then it still fails at runtime so 🤷‍♂

[shargon]

If is fast, we can do it.. otherwise what could be the problem with flag, every time you will check the expected flag, so if is not included you won't do anything

[shargon]

Could you create the PR for see easy your problem?

[ixje]

If is fast, we can do it.. otherwise what could be the problem with flag, every time you will check the expected flag, so if is not included you won't do anything

I guess the only benefit would be saving computational resources by aborting VM execution on invalid data.

Could you create the PR for see easy your problem?

I can't really create a PR yet because the helper function doesn't work (see the last 2 lines of #1231 (comment))
Otherwise the PR would just become applying

if (!Enum.IsDefined(typeof(object_type), Type)) 
     throw new FormatException(); 

to all locations that are not using it but should.

[shargon]

Try with

public static class EnumHelper
    {
        public static T StrictEnumFrom<T>(int value) where T : Enum
        {
            T state = (T)Enum.ToObject(typeof(T), value);
            if (!EnumValueCache<T>.DefinedValues.Contains(state))
                throw new FormatException();
            return state;
        }

        public static T StrictEnumFrom<T>(byte value) where T : Enum
        {
            T state = (T)Enum.ToObject(typeof(T), value);
            if (!EnumValueCache<T>.DefinedValues.Contains(state))
                throw new FormatException();
            return state;
        }

        private static class EnumValueCache<T> where T : Enum
        {
            public static readonly HashSet<T> DefinedValues = new HashSet<T>((T[])Enum.GetValues(typeof(T)));
        }
    }

[shargon]

Please do some benchmarks with Enum.IsDefined(typeof(T), value); instead of your hashset

[ixje]

Don't mind my C# benchmark skills 😅

        private static void func1(int i)
        {
            VMState state = EnumHelper.StrictFrom<VMState>(i);
        }

        private static void func2(int i)
        {
            VMState value = (VMState)i; 
            if (!Enum.IsDefined(typeof(VMState), value))
                throw new FormatException();
        }

            var watch = new Stopwatch();
            List<int> indice = new List<int> { 0, 1, 2, 4 };
            // clean up
            GC.Collect();
            GC.WaitForPendingFinalizers();
            GC.Collect();

            watch.Start();
            for (int i = 0; i < 5000; i++)
            {
                func2(indice[i%4]);
            }
            watch.Stop();
            Console.WriteLine(" Time Elapsed {0} ms", watch.Elapsed.TotalMilliseconds);

3 runs of the above code
func1: 35.639 ms, 33.4553 ms, 33.4517 ms
func2: 74.522 ms, 76.22 ms, 66.9185 ms
So the hashset is ~2x faster

[shargon]

Any update @ixje ?
I think that it's a good idea

[ixje]

I'll try to PR this week. First have to finish some other task.

[ixje]

@shargon I wanted to implement it in NEO but I'm getting the error:

Feature 'enum generic type constraints' is not available in C# 7.0. Please use language version 7.3 or greater

So this means the whole NEO project needs to be upgraded to use C# language version 7.3 or greater. This sounds like a big change, so before I continue I'd like to hear if that is acceptable.

[shargon]

In master we are using the preview version... it's weird

neo/neo/neo.csproj

Line 20 in 8ccc5e9

<LangVersion>preview</LangVersion>