#67-2: Remove extra evaluations for bindings by introducing a cached set of bindings #69
Conversation
Update from master
Changed the bindings protection to cache an initial Secure set of bindings, which are then put into custom bindings, and also used to check whether the engine bindings need to be reset. This does not protect from malicious actors!
| @Ignore | ||
| public class TestPerformance { | ||
| @Test | ||
| public void noBindings() throws ScriptCPUAbuseException, ScriptException { |
There was a problem hiding this comment.
Would it be possible to prefix all unit test methods with test eg testNoBinding? Just for consistencies sake!
There was a problem hiding this comment.
I have checked the added tests and ensured they follow this pattern.
| for (int i = 0; i< 1000; ++i) { | ||
| sandbox.eval(js); | ||
| } | ||
| System.out.println("No binding " + (((double)(System.nanoTime() - start))/1_000_000_000)); |
There was a problem hiding this comment.
Could the output to std out be replaced with asserts? e.g. Assert.assertTrue(time < ...)
There was a problem hiding this comment.
I have opted to remove the test file entirely, it was mostly for my own purposes to see what the difference was between the implementations. I agree they are useless tests, and any asserts would be flaky and prone to failure. They will exist in the history if anyone wants to revive them for some purpose.
| } | ||
|
|
||
| // This will detect whether the current engine bindings match the cached protected bindings | ||
| private boolean engineBindingsSafe() { |
There was a problem hiding this comment.
Could we rename to engineBindingsUnchanged?
There was a problem hiding this comment.
I have renamed it as suggested, it is much clearer this way.
|
@mxro Thank you very much for the comments, I have addressed them as best I can. Let me know if you have any other thoughts. |
|
@everestbt Perfect, thank you! Sorry again for merging this in later. Merged to master now but not deployed in a release yet. |
This works by performing the following:
Additional parts done here, if before a script is run it detects that the secure settings have been changed inside the engine then it will re-create them. I am not certain this is needed as this doesn't protect a single running script but it will stop one script from causing problems for another. To address this fully further sanitation of the scripts going in would be needed but that seems outside the scope.
Typically the cached bindings only contains 16 entries so this check should not cost anything but the reset of the bindings does cost something, however it should rarely take place unless someone makes a mistake.
Important to note this issue discovered for binding still exists for ScriptContext, but at the moment I don't have time to dig into this. I will open an issue to flag this after this pull request and the current issue is closed.
From my testing this resolves issue #67, any comments on the code would be appreciated.