From 253e4cbb73ca62045269e1ad566f5c4f23fa69ca Mon Sep 17 00:00:00 2001
From: "Anthony J. Thibault" <tony@highfidelity.io>
Date: Thu, 14 Apr 2016 16:25:17 -0700
Subject: [PATCH] validate arguments to MyAvatar.addAnimationStateHandler()

Also validate arguments to MyAvatar.removeAnimationStateHandler() and the return result from the user provided callback function.
---
 libraries/animation/src/Rig.cpp              | 56 +++++++++++++++-----
 libraries/script-engine/src/ScriptEngine.cpp |  8 ++-
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/libraries/animation/src/Rig.cpp b/libraries/animation/src/Rig.cpp
index 6a8f190808..6d29534f36 100644
--- a/libraries/animation/src/Rig.cpp
+++ b/libraries/animation/src/Rig.cpp
@@ -783,25 +783,55 @@ void Rig::computeMotionAnimationState(float deltaTime, const glm::vec3& worldPos
     _lastVelocity = workingVelocity;
 }
 
+static bool isListOfStrings(const QScriptValue& arg) {
+    if (!arg.isArray())
+        return false;
+
+    auto lengthProperty = arg.property("length");
+    if (!lengthProperty.isNumber())
+        return false;
+
+    int length = lengthProperty.toInt32();
+    for (int i = 0; i < length; i++) {
+        if (!arg.property(i).isString()) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 // Allow script to add/remove handlers and report results, from within their thread.
 QScriptValue Rig::addAnimationStateHandler(QScriptValue handler, QScriptValue propertiesList) { // called in script thread
-    QMutexLocker locker(&_stateMutex);
-    // Find a safe id, even if there are lots of many scripts add and remove handlers repeatedly.
-    while (!_nextStateHandlerId || _stateHandlers.contains(_nextStateHandlerId)) { // 0 is unused, and don't reuse existing after wrap.
-      _nextStateHandlerId++;
+
+    // validate argument types
+    if (handler.isFunction() && (isListOfStrings(propertiesList) || propertiesList.isUndefined() || propertiesList.isNull())) {
+        QMutexLocker locker(&_stateMutex);
+        // Find a safe id, even if there are lots of many scripts add and remove handlers repeatedly.
+        while (!_nextStateHandlerId || _stateHandlers.contains(_nextStateHandlerId)) { // 0 is unused, and don't reuse existing after wrap.
+            _nextStateHandlerId++;
+        }
+        StateHandler& data = _stateHandlers[_nextStateHandlerId];
+        data.function = handler;
+        data.useNames = propertiesList.isArray();
+        if (data.useNames) {
+            data.propertyNames = propertiesList.toVariant().toStringList();
+        }
+        return QScriptValue(_nextStateHandlerId); // suitable for giving to removeAnimationStateHandler
+    } else {
+        qCWarning(animation) << "Rig::addAnimationStateHandler invalid arguments, expected (function, string[])";
+        return QScriptValue(QScriptValue::UndefinedValue);
     }
-    StateHandler& data = _stateHandlers[_nextStateHandlerId];
-    data.function = handler;
-    data.useNames = propertiesList.isArray();
-    if (data.useNames) {
-        data.propertyNames = propertiesList.toVariant().toStringList();
-    }
-    return QScriptValue(_nextStateHandlerId); // suitable for giving to removeAnimationStateHandler
 }
 
 void Rig::removeAnimationStateHandler(QScriptValue identifier) { // called in script thread
-    QMutexLocker locker(&_stateMutex);
-    _stateHandlers.remove(identifier.isNumber() ? identifier.toInt32() : 0); // silently continues if handler not present. 0 is unused
+    // validate arguments
+    if (identifier.isNumber()) {
+        QMutexLocker locker(&_stateMutex);
+        _stateHandlers.remove(identifier.toInt32()); // silently continues if handler not present. 0 is unused
+    } else {
+        qCWarning(animation) << "Rig::removeAnimationStateHandler invalid argument, expected a number";
+    }
 }
 
 void Rig::animationStateHandlerResult(int identifier, QScriptValue result) { // called synchronously from script
diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp
index a3e0744b46..506bdb9463 100644
--- a/libraries/script-engine/src/ScriptEngine.cpp
+++ b/libraries/script-engine/src/ScriptEngine.cpp
@@ -784,7 +784,13 @@ void ScriptEngine::callAnimationStateHandler(QScriptValue callback, AnimVariantM
     callingArguments << javascriptParameters;
     assert(currentEntityIdentifier.isInvalidID()); // No animation state handlers from entity scripts.
     QScriptValue result = callback.call(QScriptValue(), callingArguments);
-    resultHandler(result);
+
+    // validate result from callback function.
+    if (result.isValid() && result.isObject()) {
+        resultHandler(result);
+    } else {
+        qCWarning(scriptengine) << "ScriptEngine::callAnimationStateHandler invalid return argument from callback, expected an object";
+    }
 }
 
 void ScriptEngine::timerFired() {