From 253e4cbb73ca62045269e1ad566f5c4f23fa69ca Mon Sep 17 00:00:00 2001 From: "Anthony J. Thibault" Date: Thu, 14 Apr 2016 16:25:17 -0700 Subject: [PATCH 1/5] 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() { From 32e9c49cf0ab2ce1d9d1fbed63cfc8978608d75e Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Thu, 14 Apr 2016 13:50:36 -0700 Subject: [PATCH 2/5] Fix cache tallies for uncached resources --- libraries/networking/src/ResourceCache.cpp | 32 +++++++++++++--------- libraries/networking/src/ResourceCache.h | 1 + 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index c5b7db8ed1..e83272eecb 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -53,7 +53,7 @@ void ResourceCache::refresh(const QUrl& url) { if (!resource.isNull()) { resource->refresh(); } else { - _resources.remove(url); + removeResource(url); resetResourceCounters(); } } @@ -140,11 +140,8 @@ void ResourceCache::addUnusedResource(const QSharedPointer& resource) // If it doesn't fit or its size is unknown, remove it from the cache. if (resource->getBytes() == 0 || resource->getBytes() > _unusedResourcesMaxSize) { resource->setCache(nullptr); - - _totalResourcesSize -= resource->getBytes(); - _resources.remove(resource->getURL()); + removeResource(resource->getURL(), resource->getBytes()); resetResourceCounters(); - return; } reserveUnusedResource(resource->getBytes()); @@ -173,8 +170,7 @@ void ResourceCache::reserveUnusedResource(qint64 resourceSize) { it.value()->setCache(nullptr); auto size = it.value()->getBytes(); - _totalResourcesSize -= size; - _resources.remove(it.value()->getURL()); + removeResource(it.value()->getURL(), size); _unusedResourcesSize -= size; _unusedResources.erase(it); @@ -198,6 +194,11 @@ void ResourceCache::resetResourceCounters() { emit dirty(); } +void ResourceCache::removeResource(const QUrl& url, qint64 size) { + _resources.remove(url); + _totalResourcesSize -= size; +} + void ResourceCache::updateTotalSize(const qint64& oldSize, const qint64& newSize) { _totalResourcesSize += (newSize - oldSize); emit dirty(); @@ -370,12 +371,12 @@ void Resource::refresh() { } void Resource::allReferencesCleared() { - if (_cache && isCacheable()) { - if (QThread::currentThread() != thread()) { - QMetaObject::invokeMethod(this, "allReferencesCleared"); - return; - } + if (QThread::currentThread() != thread()) { + QMetaObject::invokeMethod(this, "allReferencesCleared"); + return; + } + if (_cache && isCacheable()) { // create and reinsert new shared pointer QSharedPointer self(this, &Resource::allReferencesCleared); setSelf(self); @@ -383,8 +384,13 @@ void Resource::allReferencesCleared() { // add to the unused list _cache->addUnusedResource(self); - } else { + if (_cache) { + // remove from the cache + _cache->removeResource(getURL(), getBytes()); + _cache->resetResourceCounters(); + } + deleteLater(); } } diff --git a/libraries/networking/src/ResourceCache.h b/libraries/networking/src/ResourceCache.h index 84eba1cdc0..c1f99ff1c7 100644 --- a/libraries/networking/src/ResourceCache.h +++ b/libraries/networking/src/ResourceCache.h @@ -153,6 +153,7 @@ private: void reserveUnusedResource(qint64 resourceSize); void clearUnusedResource(); void resetResourceCounters(); + void removeResource(const QUrl& url, qint64 size = 0); QHash> _resources; int _lastLRUKey = 0; From 089e7eb6b33b0481f053ed87db22b51589cbd7ef Mon Sep 17 00:00:00 2001 From: howard-stearns Date: Fri, 15 Apr 2016 10:17:57 -0700 Subject: [PATCH 3/5] Don't truncate time when accumulating, and then divide by non-truncated. --- libraries/shared/src/shared/RateCounter.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/shared/src/shared/RateCounter.h b/libraries/shared/src/shared/RateCounter.h index 754439ec15..e24b1e6eca 100644 --- a/libraries/shared/src/shared/RateCounter.h +++ b/libraries/shared/src/shared/RateCounter.h @@ -22,10 +22,10 @@ class RateCounter { public: void increment(size_t count = 1) { auto now = usecTimestampNow(); - auto currentIntervalMs = (uint32_t)((now - _start) / USECS_PER_MSEC); - if (currentIntervalMs > INTERVAL) { + float currentIntervalMs = (now - _start) / (float) USECS_PER_MSEC; + if (currentIntervalMs > (float) INTERVAL) { float currentCount = _count; - float intervalSeconds = (float)currentIntervalMs / (float)MSECS_PER_SECOND; + float intervalSeconds = currentIntervalMs / (float) MSECS_PER_SECOND; _rate = roundf(currentCount / intervalSeconds * _scale) / _scale; _start = now; _count = 0; From cff5886313156c97bc3a297283194fbc10cee811 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 15 Apr 2016 10:46:19 -0700 Subject: [PATCH 4/5] Use correct resources lock --- libraries/networking/src/ResourceCache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 7449f61821..96e05c8d09 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -255,7 +255,7 @@ void ResourceCache::resetResourceCounters() { } void ResourceCache::removeResource(const QUrl& url, qint64 size) { - QWriteLocker locker(&_unusedResourcesLock); + QWriteLocker locker(&_resourcesLock); _resources.remove(url); _totalResourcesSize -= size; } From 38418d016915624f7c8c247f5ea75689212e0f1d Mon Sep 17 00:00:00 2001 From: "Anthony J. Thibault" Date: Fri, 15 Apr 2016 13:46:55 -0700 Subject: [PATCH 5/5] Moved isListOfStrings into shared/ScriptValueUtils Also fixed some single line ifs. --- libraries/animation/src/Rig.cpp | 19 +------------ libraries/shared/src/ScriptValueUtils.cpp | 34 +++++++++++++++++++++++ libraries/shared/src/ScriptValueUtils.h | 21 ++++++++++++++ 3 files changed, 56 insertions(+), 18 deletions(-) create mode 100644 libraries/shared/src/ScriptValueUtils.cpp create mode 100644 libraries/shared/src/ScriptValueUtils.h diff --git a/libraries/animation/src/Rig.cpp b/libraries/animation/src/Rig.cpp index 6d29534f36..db658ef728 100644 --- a/libraries/animation/src/Rig.cpp +++ b/libraries/animation/src/Rig.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "AnimationLogging.h" @@ -783,24 +784,6 @@ 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 diff --git a/libraries/shared/src/ScriptValueUtils.cpp b/libraries/shared/src/ScriptValueUtils.cpp new file mode 100644 index 0000000000..e352c0546d --- /dev/null +++ b/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; +} diff --git a/libraries/shared/src/ScriptValueUtils.h b/libraries/shared/src/ScriptValueUtils.h new file mode 100644 index 0000000000..2e120a7217 --- /dev/null +++ b/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 + +bool isListOfStrings(const QScriptValue& value); + +#endif // #define hifi_ScriptValueUtils_h