Add JsInterpreter to replace Rhino usage#2812
Conversation
|
This should now be ready for review. I ran all the tests and they worked, fixed a couple things, and added a simpler API. |
…ange +more tests for some changed functionality
fire-light42
left a comment
There was a problem hiding this comment.
It looks like the JS interpreter does not handle the JSFuck obfuscation technique, which abuses easily overlooked parts of standard JS to obfuscate code, it should be very easy to generate test cases.
Would it be possible to support this or would it be too much scope creep? Even if it cannot be fully supported it might reveal some easy-to-fix edge cases.
|
It should be possible to support it although I am not entirely sure how at the moment. I aimed for this to support a lot of the standard JS spec, and didn't really go beyond it. I tried to support everything I found extensions using Rhino for and added test cases for some of those uses. I think if we do support it, we can, but perhaps should be a separate PR as that is kinda a different scope then what this was aiming for but up to you, I could look more into this this weekend though if you want. |
|
I will also do some testing on the real-world usage of the JS helpers to determine if this is required. Currently it does not seem like a blocker. |
|
Supporting this does expose other edge case bugs here. I have a patch I will push soon that will support this, fix numerous other things, and add more than 150 new test cases for the edge case bugs and JSFuck format. |
|
I do want to note also, I did not want to reinvente JavaScript here so this definitely does not support everything, if something needs some other support added we can add it later. I decided to add support for instanceof in my recent commit here only because I found that one could have some use for some scripts potentially and we support typeof. But it still doesn't support some things like some less used Math functions, static functions like Array.isArray, Array.of, etc.... I could easily add it later if it's something we need or want though. Things like console.* are just silently swallowed no-ops so they don't cause noise but also don't fail the scripts. This can all be changed later if necessary. This is designed to be lighter weight, so I didn't want to do everything. |
fire-light42
left a comment
There was a problem hiding this comment.
Everything looks good, the only real issue is a possible StackOverflowError. We can catch it easily, but not sure if we can (or should) mitigate it more.
After this I would say it's ready to merge.
fire-light42
left a comment
There was a problem hiding this comment.
I tested this with the "js is weird" website. While it is not expected to pass all these tests, it might be interesting to check out some edge cases you might have forgotten about like octal (0123) and pow (**) if it is not too much of a bother.
However, I really wanted to ask for a way to limit the execution of JS to something reasonable like 5s, or x number of instructions, as otherwise we might burn the cpu if they have some infinite loop.
@Test
fun inf() {
// We should be handle to exit long-running tasks by quiting
assertEquals(evalJs("while(true){}"), Unit)
}The main reason for this is that we use withTimeout to cancel long running jobs, but this implementation will not acknowledge this behavior and burn the cpu until restart. Most/all of the time JS is not written by the developer, but an untrusted source, so we can not expect good code.
@Test
fun inf_suspend() {
runBlocking {
// We should be handle to exit long-running tasks by quiting
withTimeout(100) {
evalJs("while(true){}")
}
assertEquals(1, 1)
}
} @Test
fun weird1() {
assertEquals(1.0, num("true + false"))
}
@Test
fun weird2() {
assertEquals(3.0, num("[,,,].length"))
}
@Test
fun weird2_2() {
assertEquals(2.0, num("[1,2,].length"))
}
@Test
fun weird2_3() {
assertEquals(3.0, num("[1,2,,].length"))
}
@Test
fun weird3() {
assertEquals("1,2,34,5,6", evalJs("[1, 2, 3] + [4, 5, 6]"))
}
@Test
fun weird4() {
assertEquals(2.0, num("10,2"))
}
@Test
fun weird5() {
assertEquals(false,evalJs("!!\"\""))
}
@Test
fun weird6() {
assertEquals(1.0, num("+!![]"))
}
@Test
fun weird7() {
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
assertEquals(5.0, num("parseInt(0.0000005)"))
}
@Test
fun weird8() {
assertEquals(false, evalJs("true == \"true\""))
}
@Test
fun weird9() {
// Octal
assertEquals(5.0, num("010 - 03"))
}
@Test
fun weird10() {
assertEquals(0.0, evalJs("\"\" - - \"\""))
}
@Test
fun weird11() {
assertEquals(0.0, num("null + 0"))
}
@Test
fun weird12() {
assertEquals(num("NaN"), evalJs("0/0"))
}
@Test
fun weird13() {
// Pow
assertEquals(num("Infinity"), evalJs("10 ** 1000"))
assertEquals(false, evalJs("1/0 > 10 ** 1000"))
}
@Test
fun weird14() {
// inc vs add
assertEquals(2.0, evalJs("true+1"))
assertEquals(Unit, evalJs("true++"))
}
@Test
fun weird15() {
assertEquals(-1.0, evalJs("\"\" - 1"))
}
@Test
fun weird16() {
assertEquals("00", evalJs("(null - 0) + \"0\""))
}
@Test
fun weird17() {
assertEquals(Double.NaN, evalJs("true + (\"true\" - 0) "))
}
@Test
fun weird18() {
assertEquals(0.0, evalJs("!5 + !5"))
}
@Test
fun weird19() {
assertEquals("", evalJs("[] + []"))
}
@Test
fun weird20() {
assertEquals("33", evalJs("1 + 2 + \"3\""))
}
@Test
fun weird21() {
assertEquals("number", evalJs("typeof NaN"))
}
@Test
fun weird22() {
assertEquals(Double.NaN, evalJs("undefined + false"))
}
@Test
fun weird23() {
assertEquals("", evalJs("\"\" && -0"))
}
@Test
fun weird24() {
assertEquals(0.0, evalJs("+!!NaN * \"\" - - [,]"))
}And you passed a supprisingly large amount, with only these failures
weird2
Expected :3.0
Actual :2.0
weird7
Expected :5.0
Actual :NaN
weird9
Expected :5.0
Actual :7.0
weird10
Expected :0.0
Actual :NaN
weird13
Expected :Infinity
Actual :1000.0
weird14
Expected :kotlin.Unit
Actual :1.0
weird15
Expected :-1.0
Actual :NaN
weird22
Expected :NaN
Actual :0.0
weird24
Expected :0.0
Actual :NaN
|
I will work on this more tomorrow, though I potentially if you are okay with it, focus on just the timeout stuff, and we can later expand to fix some edge cases in a followup. Though I will try and fix some of them + add tests, just a follow up instead if something is more complicated is what I would prefer if you're okay with it. |
Yes, the timeout issue is the only real blocker I can see. |
I went ahead and fixed all the edge cases you mentioned above as well as added limiting via budget either instruction count or timeout. I think I got it all working and added numerous more tests for it. Hopefully I didn't miss anything else now though. |
I added 367 test cases for this, all of which pass locally. When adding tests I specifically targeted some common uses of rhino by extensions, and additionally added things extensions may want or need to use in the future. We can also expand those tests should something else come up in the future.