DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
434 stars 59 forks source link

Support unlimited use of memory and stack. #272

Closed ahaoboy closed 2 months ago

ahaoboy commented 4 months ago

In some situations, we do not want to restrict the memory and stack size of a program because we do not know the specific values.

// Setting the limit to 0 is equivalent to unlimited memory.
let rt = Runtime::new().unwrap();
rt.set_memory_limit(0);
// error
let ctx = Context::full(&rt).unwrap();

Perhaps we can add two functions, unlimit_memory and unlimit_stack, to explicitly indicate. Compared to setting 0 as the maximum, this approach provides better semantic clarity.

DelSkayn commented 4 months ago

Is see that there is a comment on the set_memory_limit function that says that setting it to 0 removes the limit. I don't really know why that is there, as it is just not true. There isn't really a way to disable memory checks for both the heap or the stack in QuickJS. So I think it is better to keep the functions the way they are as they reflect the fact that you can't disable it. If you really want to never hit the limit you can always set the limit to usize::MAX.

I will remove the comment about 0 disabling the limit though.

ahaoboy commented 4 months ago

Use rt.set_memory_limit(usize::MAX) over rt.unlimit_memory(), the latter is more semantic, the former requires the use of usize which may not be the same on different platforms and the type of size_t. Also the set_memory_limit function fails to create a context if passed a very small value such as 256.

DelSkayn commented 2 months ago

the use of usize seems fine to me as long as it is properly truncated when size_t is smaller then usize. If the size_t is bigger then usize, usize should still be able to fit the size of addressable memory so if you set it to usize::MAX it should always be effectively unlimited. If size_t is smaller then the usize quickjs is unable to set a larger limit anyway.

Regarding setting it to a low value failing to create a context. I think that is fine as long as it isn't causing any segfaults.