Merge pull request #27 in XK/xenko from virgile/safe_oob_update_engin…

…e to master-1.9

* commit 'a30f222602b8c93f709fe5fb430cb2f6717121df':
  [UpdateEngine] Skip update if array/list is not big enough (which would otherwise cause memory corruption or exception)

sources/engine/SiliconStudio.Xenko.Engine.Tests/TestUpdateEngine.cs

@@ -365,6 +365,57 @@ public void TestNullSkip()
             Assert.That(test.IntProperty, Is.EqualTo(456));
         }
 
+        [Test]
+        public void TestOutOfBoundsSkip()
+        {
+            var test = new TestClass
+            {
+                TestClassArray =
+                {
+                    [0] = new TestClass(),
+                    [1] = new TestClass()
+                },
+                TestClassList =
+                {
+                    [0] = new TestClass(),
+                    [1] = new TestClass()
+                }
+            };
+
+            // Check that ctor of TestClass properly initialized size of array/list to 4 (this test relies on it)
+            Assert.That(test.IntArray.Length, Is.EqualTo(4));
+            Assert.That(test.IntList.Count, Is.EqualTo(4));
+            Assert.That(test.TestClassArray.Length, Is.EqualTo(2));
+            Assert.That(test.TestClassList.Count, Is.EqualTo(2));
+
+            UpdateEngine.RegisterMemberResolver(new ArrayUpdateResolver<int>());
+            UpdateEngine.RegisterMemberResolver(new ListUpdateResolver<int>());
+
+            // Combine many of the other tests in one, to easily test if switching from one member to another works well
+            var updateMemberInfo = new List<UpdateMemberInfo>
+            {
+                new UpdateMemberInfo("IntArray[0]", 0),
+                new UpdateMemberInfo("IntArray[4]", 0),
+                new UpdateMemberInfo("IntList[0]", 0),
+                new UpdateMemberInfo("IntList[4]", 0),
+                new UpdateMemberInfo("TestClassArray[0].IntField", 0),
+                new UpdateMemberInfo("TestClassArray[2].IntField", 0),
+                new UpdateMemberInfo("TestClassList[0].IntField", 0),
+                new UpdateMemberInfo("TestClassList[2].IntField", 0),
+            };
+
+            var blittableData = new TestData[] { 123 };
+
+            // This shouldn't crash
+            RunUpdateEngine(test, updateMemberInfo, blittableData, null);
+
+            // Update shouldn't have been done (we skip the whole stuff if it goes out of bound)
+            Assert.That(test.IntArray[0], Is.EqualTo(0));
+            Assert.That(test.IntList[0], Is.EqualTo(0));
+            Assert.That(test.TestClassArray[0].IntField, Is.EqualTo(0));
+            Assert.That(test.TestClassList[0].IntField, Is.EqualTo(0));
+        }
+
         private static unsafe void RunUpdateEngine(TestClass test, List<UpdateMemberInfo> updateMemberInfo, TestData[] blittableData, UpdateObjectData[] objectData)
         {
             var compiledUpdate = UpdateEngine.Compile(test.GetType(), updateMemberInfo);

sources/engine/SiliconStudio.Xenko.Engine/SiliconStudio.Xenko.Engine.csproj

@@ -2627,7 +2627,10 @@
     <Compile Include="Updater\BlittableHelper.cs" />
     <Compile Include="Updater\CompiledUpdate.cs" />
     <Compile Include="Updater\DataMemberUpdatableAttribute.cs" />
+    <Compile Include="Updater\EnterChecker.cs" />
+    <Compile Include="Updater\ListEnterChecker.cs" />
     <Compile Include="Updater\ListUpdateResolver.cs" />
+    <Compile Include="Updater\UpdatableArrayAccessor.cs" />
     <Compile Include="Updater\UpdatableCustomAccessor.cs" />
     <Compile Include="Updater\UpdatableField.cs" />
     <Compile Include="Updater\UpdatableFieldT.cs" />

sources/engine/SiliconStudio.Xenko.Engine/Updater/ArrayUpdateResolver.cs

@@ -18,10 +18,7 @@ public override UpdatableMember ResolveIndexer(string indexerName)
             if (!int.TryParse(indexerName, NumberStyles.Any, CultureInfo.InvariantCulture, out indexerValue))
                 throw new InvalidOperationException(string.Format("Property path parse error: could not parse indexer value '{0}'", indexerName));
 
-            var updatableField = new UpdatableField<T>(0);
-
-            var offset = UpdateEngineHelper.ArrayFirstElementOffset + indexerValue * updatableField.Size;
-            updatableField.Offset = offset;
+            var updatableField = new UpdatableArrayAccessor<T>(indexerValue);
 
             return updatableField;
         }

sources/engine/SiliconStudio.Xenko.Engine/Updater/EnterChecker.cs

@@ -0,0 +1,15 @@
+namespace SiliconStudio.Xenko.Updater
+{
+    /// <summary>
+    /// Provides a way to perform additional checks when entering an object (typically out of bounds checks).
+    /// </summary>
+    public abstract class EnterChecker
+    {
+        /// <summary>
+        /// Called by <see cref="UpdateEngine.Run"/> to perform additional checks when entering an object (typically out of bounds checks).
+        /// </summary>
+        /// <param name="obj">The object being entered.</param>
+        /// <returns>True if checks succeed, false otherwise.</returns>
+        public abstract bool CanEnter(object obj);
+    }
+}

sources/engine/SiliconStudio.Xenko.Engine/Updater/ListEnterChecker.cs

@@ -0,0 +1,23 @@
+using System.Collections.Generic;
+
+namespace SiliconStudio.Xenko.Updater
+{
+    /// <summary>
+    /// Implementation of <see cref="EnterChecker"/> for <see cref="IList{T}"/>.
+    /// </summary>
+    class ListEnterChecker<T> : EnterChecker
+    {
+        private readonly int minimumCount;
+
+        public ListEnterChecker(int minimumCount)
+        {
+            this.minimumCount = minimumCount;
+        }
+
+        /// <inheritdoc/>
+        public override bool CanEnter(object obj)
+        {
+            return minimumCount <= ((IList<T>)obj).Count;
+        }
+    }
+}

sources/engine/SiliconStudio.Xenko.Engine/Updater/UpdatableArrayAccessor.cs

@@ -0,0 +1,23 @@
+namespace SiliconStudio.Xenko.Updater
+{
+    /// <summary>
+    /// Defines how to get and set an array value for the <see cref="UpdateEngine"/>.
+    /// </summary>
+    public class UpdatableArrayAccessor<T> : UpdatableField<T>
+    {
+        public UpdatableArrayAccessor(int index) : base(0)
+        {
+            Offset = UpdateEngineHelper.ArrayFirstElementOffset + index * Size;
+        }
+
+        /// <inheritdoc/>
+        public override EnterChecker CreateEnterChecker()
+        {
+            // Compute index
+            var index = (Offset - UpdateEngineHelper.ArrayFirstElementOffset)/Size;
+
+            // Expect an array at least index + 1 items
+            return new ListEnterChecker<T>(index + 1);
+        }
+    }
+}

sources/engine/SiliconStudio.Xenko.Engine/Updater/UpdatableListAccessor.cs

@@ -126,5 +126,12 @@ ldarg data
 #endif
             throw new NotImplementedException();
         }
+
+        /// <inheritdoc/>
+        public override EnterChecker CreateEnterChecker()
+        {
+            // Expect a list at least index + 1 items
+            return new ListEnterChecker<T>(Index + 1);
+        }
     }
 }

sources/engine/SiliconStudio.Xenko.Engine/Updater/UpdatableMember.cs

@@ -11,5 +11,14 @@ public abstract class UpdatableMember
         /// Gets the type of the member.
         /// </summary>
         public abstract Type MemberType { get; }
+
+        /// <summary>
+        /// Called by <see cref="UpdateEngine.Compile"/> to generate additional checks when entering an object (typically out of bound checks).
+        /// </summary>
+        /// <returns>The created enter checker (or null if not needed).</returns>
+        public virtual EnterChecker CreateEnterChecker()
+        {
+            return null;
+        }
     }
 }

sources/engine/SiliconStudio.Xenko.Engine/Updater/UpdateEngine.cs

@@ -90,12 +90,34 @@ public AnimationBuilderStackEntry(Type type, int startIndex, int endIndex, int o
         /// </summary>
         struct ComputeUpdateOperationState
         {
+            /// <summary>
+            /// Current list of update operations being built.
+            /// </summary>
             public List<UpdateOperation> UpdateOperations;
+
+            /// <summary>
+            /// Stack of the current path.
+            /// </summary>
             public List<AnimationBuilderStackEntry> StackPath;
+
             public int NewOffset;
             public int PreviousOffset;
+
+            /// <summary>
+            /// Current path start index in the string.
+            /// </summary>
             public int ParseElementStart;
+
+            /// <summary>
+            /// Current path end index in the string.
+            /// </summary>
             public int ParseElementEnd;
+
+            /// <summary>
+            /// Contains the last node that was just left.
+            /// Used to call <see cref="UpdatableMember.CreateEnterChecker"/>.
+            /// </summary>
+            public UpdatableMember LastChildMember;
         }
 
         /// <summary>
@@ -315,13 +337,24 @@ private static void PopObjects(ref ComputeUpdateOperationState state, int desire
                     state.PreviousOffset = stackPathPart.LeaveOffset;
                 }
 
-                // Sets how many operations to skip in case the object we entered was null
                 if (stackPathPart.OperationIndex != -1)
                 {
                     var updateOperation = state.UpdateOperations[stackPathPart.OperationIndex];
+
+                    // Setup VerifyEnter using last child of current node
+                    var verifyEnter = state.LastChildMember?.CreateEnterChecker();
+                    if (verifyEnter != null)
+                    {
+                        updateOperation.EnterChecker = verifyEnter;
+                    }
+
+                    // Sets how many operations to skip in case the object we entered was null
                     updateOperation.SkipCountIfNull = state.UpdateOperations.Count - stackPathPart.OperationIndex - 1;
                     state.UpdateOperations[stackPathPart.OperationIndex] = updateOperation;
                 }
+
+                // Set last child member to the node we just left
+                state.LastChildMember = stackPathPart.Member;
             }
         }
 
@@ -330,6 +363,10 @@ private static void ProcessMember(ref ComputeUpdateOperationState state, UpdateM
             int leaveOffset = 0;
             var leaveOperation = UpdateOperationType.Invalid;
 
+            // Note: only matters on leaf/leave nodes
+            // It will be processed during following PopObjects
+            state.LastChildMember = updatableMember;
+
             var updatableField = updatableMember as UpdatableField;
             if (updatableField != null)
             {
@@ -465,7 +502,7 @@ public static void Run(object target, CompiledUpdate compiledUpdate, IntPtr upda
                     case UpdateOperationType.EnterObjectProperty:
                     {
                         nextObject = ((UpdatableProperty)operation.Member).GetObject(currentPtr);
-                        if (nextObject == null && operation.SkipCountIfNull != -1)
+                        if ((nextObject == null || (!operation.EnterChecker?.CanEnter(nextObject) ?? false)) && operation.SkipCountIfNull != -1)
                         {
                             index += operation.SkipCountIfNull;
                             operation = Interop.AddPinned(operation, operation.SkipCountIfNull);
@@ -500,7 +537,7 @@ public static void Run(object target, CompiledUpdate compiledUpdate, IntPtr upda
                     case UpdateOperationType.EnterObjectField:
                     {
                         nextObject = ((UpdatableField)operation.Member).GetObject(currentPtr);
-                        if (nextObject == null && operation.SkipCountIfNull != -1)
+                        if ((nextObject == null || (!operation.EnterChecker?.CanEnter(nextObject) ?? false)) && operation.SkipCountIfNull != -1)
                         {
                             index += operation.SkipCountIfNull;
                             operation = Interop.AddPinned(operation, operation.SkipCountIfNull);
@@ -521,7 +558,7 @@ public static void Run(object target, CompiledUpdate compiledUpdate, IntPtr upda
                     case UpdateOperationType.EnterObjectCustom:
                     {
                         nextObject = ((UpdatableCustomAccessor)operation.Member).GetObject(currentPtr);
-                        if (nextObject == null && operation.SkipCountIfNull != -1)
+                        if ((nextObject == null || (!operation.EnterChecker?.CanEnter(nextObject) ?? false)) && operation.SkipCountIfNull != -1)
                         {
                             index += operation.SkipCountIfNull;
                             operation = Interop.AddPinned(operation, operation.SkipCountIfNull);

sources/engine/SiliconStudio.Xenko.Engine/Updater/UpdateOperation.cs

@@ -7,6 +7,7 @@ struct UpdateOperation
     {
         internal UpdateOperationType Type;
         internal UpdatableMember Member;
+        internal EnterChecker EnterChecker;
 
         // TODO: Should we switch to short + short? (note: could be a problem with big arrays)