search

rubyjs / therubyracer

Embed the V8 Javascript Interpreter into Ruby
1.66k stars 192 forks source link

Added support for context eval timeouts #280

Closed SamSaffron closed 10 years ago

SamSaffron commented 10 years ago

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 commented 10 years ago

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

SamSaffron commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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

SamSaffron commented 10 years ago

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 commented 10 years ago

@cowboyd added protection :)

cowboyd commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

SamSaffron commented 10 years ago

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/cowboyd/therubyracer/pull/280#issuecomment-31513213 .