Added support for context eval timeouts

[SamSaffron]

This implements #279 without it there is no way to have built in DoS protection.

Usage:

ctx = V8::Context.new(:timeout => 10)
lambda {ctx.eval("while(true){}")}.should(raise_error)

Timeout is in milliseconds

[SamSaffron]

error is due to rbx, no idea what is going on there. rbx is broke on travis.

[SamSaffron]

@cowboyd did you have a chance to look at this, I have Discourse deployed in production on this fork. needed to add safety in case rogue js found its way into our app.

[cowboyd]

Thanks for this @SamSaffron, this will make a lot of people very happy. I am on vacation at the moment, and will review this when I get back. In the meantime, not being overly familiar with the semantics of pthread_cancel(), what will happen if it happens in the middle of v8::V8::TerminateExecution? Do we risk any corruption of the VM?

[cowboyd]

also, I will update the .travis.yml to better versions of rubinius.

[SamSaffron]

No worries, enjoy your holidays!

Do we risk any corruption of the VM?

I am unsure, it depends on v8, I think we can add a http://man7.org/linux/man-pages/man3/pthread_setcancelstate.3.html to ensure the thread can not be cancelled during that operation. It is possible v8 already does that.

[SamSaffron]

@cowboyd added protection :)

[cowboyd]

I tried also implementing this with v8::internal::Thread, but couldn't get it to work.... the rationale being it wouldn't depend on pthreads and would be compatible with Windows at some point in the future. That's not a huge priority for me, but something I at least wanted to try. I couldn't seem to get the forward declaration right. It might be worth a look 39f9760

That said, a working implementation is worth 1000 hypotheticals and this seems to be rock solid.

What about API. Do you think that timeout should be specified on the context level or on the individual calls to eval, or both? Perhaps eval("while (true) {}", "file.js", 1, timeout: 10)

[SamSaffron]

API. Do you think that timeout should be specified on the context level or on the individual calls to eval, or both?

Personally I feel this should be an option on the context, there is a minor additional overhead for timeout support and you would generally add this protection for you contexts that need the feature.

I am fine for it to be included in both, don't see it causing any harm. I definitely want it on the context level cause it makes it simpler to code against. That way you don't need to remember to add a timeout in 50 spots.

Regarding threading stuff, it may be possible to use v8 to timeout the context by spinning a second v8 context, I saw some of that stuff done internally in the tests (dig up the multi threaded ones). It looked very complicated to me but doable and portable. That said, I would not have Windows as a major priority here.

[cowboyd]

Yes, I suppose if somebody has a compelling use case for varying the timeout of evals within the same context, it can always be added later.

[cowboyd]

Thanks! Can you please review the documentation I added with d51befb?

[SamSaffron]

Documentation looks perfect :) thanks heaps

On Fri, Jan 3, 2014 at 8:44 PM, Charles Lowell notifications@github.comwrote:

Thanks! Can you please review the documentation I added with d51befbhttps://github.com/cowboyd/therubyracer/commit/d51befbb8eb6a7c79585a9eed2b8f2708bf04491
?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/280#issuecomment-31513213
.