Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding multiple callbacks causes Err(Exception(String("SyntaxError: stack overflow"))) #66

Closed
jrop opened this issue May 7, 2020 · 6 comments

Comments

@jrop
Copy link

jrop commented May 7, 2020

If I add multiple callbacks and then call ctx.eval(...), no matter what I put in the eval statement, it causes a fatal error: Err(Exception(String("SyntaxError: stack overflow"))). See the following example to reproduce.

use quick_js::{Context, ContextError, JsValue};

fn qjs_add(x: i32, y: i32) -> i32 {
    x + y
}
fn qjs_sleep(ms: i32) -> JsValue {
    std::thread::sleep(std::time::Duration::from_millis(ms as u64));
    JsValue::Null
}

fn main() {
    let ctx = Context::new().unwrap();
    ctx.add_callback("qjs_add", qjs_add).unwrap();
    ctx.add_callback("qjs_sleep", qjs_sleep).unwrap();

    let result = ctx.eval(";").unwrap();
}

Causes the following error:

Exception(String("SyntaxError: stack overflow"))thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Exception(String("SyntaxError: stack overflow"))', src/main.rs

This error shows up no matter what I put in ctx.eval("..."), unless I limit the calls to add_callback(...) to 1. Is there a limit of one callback?

@jrop
Copy link
Author

jrop commented May 7, 2020

Somehow I misdiagnosed the issue:
If I change:

pub fn ctx() -> Result<Context, Box<dyn Error>> {
    let ctx = Context::new()?;
    ctx.add_callback("qjs_add", qjs_add)?;
    ctx.add_callback("qjs_sleep", qjs_sleep)?;
    Ok(ctx)
}

To:

pub fn ctx() -> Result<Context, Box<dyn Error>> {
    let ctx = Context::new()?;
    ctx.add_callback("qjs_add", qjs_add).unwrap();
    ctx.add_callback("qjs_sleep", qjs_sleep).unwrap();
    Ok(ctx)
}

It works. I am not sure why, but calling unwrap fixes the issue.

@jrop jrop closed this as completed May 7, 2020
@theduke theduke reopened this May 7, 2020
@theduke
Copy link
Owner

theduke commented May 25, 2020

Can you post a full reproduction of the error?

Using your fn ctx() -> Result<..> {} works fine for me.

@jrop
Copy link
Author

jrop commented May 25, 2020

I don't have the offending code anymore, sorry. Perhaps it was an environmental issue. I did code it late at night, so it's not out of the question that I missed something.

@theduke theduke closed this as completed May 25, 2020
@theduke
Copy link
Owner

theduke commented May 25, 2020

Perhaps it was an environmental issue.

Let's hope so.

Feel free to reopen if you stumble over it again.

@monoclex
Copy link

monoclex commented May 1, 2022

I have ran into the same issue.

fn main() {
    let ctx = make_context().unwrap();
    ctx.eval("").unwrap();
}

fn make_context() -> anyhow::Result<quick_js::Context> {
    let ctx = quick_js::Context::new()?;
    ctx.add_callback("f", |_: i32| quick_js::JsValue::Undefined)?;
    ctx.add_callback("g", |_: i32| quick_js::JsValue::Undefined)?;
    Ok(ctx)
}

qjs-test.zip

> cargo r
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Exception(String("SyntaxError: stack overflow"))', src/main.rs:3:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

After some debugging, this is actually a solved issue. libquickjs-sys provides the following patch:

diff -urN quickjs-2019-07-28/quickjs.c quickjs-2019-07-28-stack-overflow-signed/quickjs.c
--- quickjs-2019-07-28/quickjs.c	2019-07-28 15:03:03.000000000 +0000
+++ quickjs-2019-07-28-stack-overflow-signed/quickjs.c	2019-08-09 20:00:03.666846091 +0000
@@ -1732,9 +1732,9 @@
 
 static inline BOOL js_check_stack_overflow(JSContext *rt, size_t alloca_size)
 {
-    size_t size;
+    ptrdiff_t size;
     size = rt->stack_top - js_get_stack_pointer();
-    return unlikely((size + alloca_size) > rt->stack_size);
+    return unlikely((size + (ptrdiff_t)alloca_size) > (ptrdiff_t)rt->stack_size);
 }
 #endif

The issue is that this patch is not applied by default. After enabling the patched feature, my code repro no longer fails.

@andrieshiemstra
Copy link
Contributor

@SirJosh3917
What i do in order to prevent the stackoverflow from mallfunctioning is to call JS_UpdateStackTop every top-most call from my EventLoop

In order to ignore the check entirely you could just call JS_SetMaxStackSize with u64::MAX (which is what i did before i used JS_UpdateStackTop)

I stumbled upon this in #92 and fixed it here HiRoFa/quickjs_es_runtime#50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants