Change: Limit total script ops that can be consumed by a list valuate #11670
Conversation
| @@ -12,9 +12,13 @@ | |||
| #include "script_controller.hpp" | |||
| #include "../../debug.h" | |||
| #include "../../script/squirrel.hpp" | |||
| #include <../squirrel/sqvm.h> | |||
There was a problem hiding this comment.
Using ../ with #include <> looks wrong to me.
There was a problem hiding this comment.
That's how both of the existing squirrel includes in /src/script/ are.
There was a problem hiding this comment.
Most likely by error. A while ago we fixed a bunch of those "" vs <> for squirrel. Guess those were missed :)
There was a problem hiding this comment.
It doesn't compile with the ../ removed.
In CMakeLists.txt there is include_directories(${CMAKE_SOURCE_DIR}/src/3rdparty/squirrel/include), but
src/script/squirrel.cpp wants includes in src/3rdparty/squirrel/squirrel/.
|
|
src/script/api/script_list.cpp
Outdated
| @@ -906,6 +918,7 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) | |||
| /* See below for explanation. The extra pop is the return value. */ | |||
| sq_pop(vm, nparam + 4); | |||
|
|
|||
| vm->_ops_till_suspend_error_threshold = backup_ops_error_threshold; | |||
There was a problem hiding this comment.
Maybe we should make a small Backup wrapper here, so it is automatically restored for us when leaving scope?
Dunno if that is actually an improvement tbh .. but missing a return is also an easy mistake made :(
JGRennison
force-pushed
the
fix-limit-list-valuate-ops
branch
from
8898575
to
1d0b40b
Compare
Motivation / Problem
Script functions called by ScriptList::Valuate are never suspended.
A single script function called by ScriptList::Valuate could therefore consume an unlimited amount of CPU time, blocking the actual game.
Description
Generate an error, killing the script, once a threshold number of ops is consumed by a (top level) valuate operation, even in the case where the function would never otherwise return.
Limitations
Using the suspend parameter of sq_call complicates the accounting of the remaining ops and the handling of the stack in the case where the limit has been reached. An attempt was made using this, but was abandoned in favour of the approach in this PR.
The existing squirrel code is mostly very ugly. In order to maintain a consistent code style care has been taken to ensure that this PR is also aesthetically displeasing.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.