Merge pull request #7670 from hyperlogic/tony/validate-anim-state-handler-args

Validate arguments to MyAvatar.addAnimationStateHandler()

libraries/animation/src/Rig.cpp

@@ -20,6 +20,7 @@
 #include <GeometryUtil.h>
 #include <NumericalConstants.h>
 #include <DebugDraw.h>
+#include <ScriptValueUtils.h>
 #include <shared/NsightHelpers.h>
 
 #include "AnimationLogging.h"
@@ -796,23 +797,35 @@ void Rig::computeMotionAnimationState(float deltaTime, const glm::vec3& worldPos
 
 // 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.
+    // validate argument types
-    while (!_nextStateHandlerId || _stateHandlers.contains(_nextStateHandlerId)) { // 0 is unused, and don't reuse existing after wrap.
+    if (handler.isFunction() && (isListOfStrings(propertiesList) || propertiesList.isUndefined() || propertiesList.isNull())) {
-      _nextStateHandlerId++;
+        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);
+    // validate arguments
-    _stateHandlers.remove(identifier.isNumber() ? identifier.toInt32() : 0); // silently continues if handler not present. 0 is unused
+    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

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() {

libraries/shared/src/ScriptValueUtils.cpp

@@ -0,0 +1,34 @@
+//
+//  ScriptValueUtils.cpp
+//  libraries/shared/src
+//
+//  Created by Anthony Thibault on 4/15/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  Utilities for working with QtScriptValues
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+#include "ScriptValueUtils.h"
+
+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;
+}

libraries/shared/src/ScriptValueUtils.h

@@ -0,0 +1,21 @@
+//
+//  ScriptValueUtils.h
+//  libraries/shared/src
+//
+//  Created by Anthony Thibault on 4/15/16.
+//  Copyright 2016 High Fidelity, Inc.
+//
+//  Utilities for working with QtScriptValues
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+#ifndef hifi_ScriptValueUtils_h
+#define hifi_ScriptValueUtils_h
+
+#include <QScriptValue>
+
+bool isListOfStrings(const QScriptValue& value);
+
+#endif // #define hifi_ScriptValueUtils_h