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

Detecting stack overflows? #386

Open
tmpfs opened this issue Oct 18, 2020 · 4 comments
Open

Detecting stack overflows? #386

tmpfs opened this issue Oct 18, 2020 · 4 comments
Labels

Comments

@tmpfs
Copy link

tmpfs commented Oct 18, 2020

Hi @sunng87 , I saw this issue from a long time ago but it seems I am still able to create stack overflows pretty easily.

I can certainly create them with helpers that call out to render_template_with_context(). I think what would be useful would be to be able to pass an existing RenderContext into render_template_with_context() in order to be able to deal with this correctly. My helper could then add a stack to the RenderContext data to detect where it has already been called and respond appropriately with a RenderError.

I realize my use case is pretty esoteric (calling render_template_with_context inside a helper) but this is the only way I have been able to structure my program logic and interface exactly the way I want it.

Would you consider modifying render_template_with_context to accept an additional Option<RenderContext> argument? Looking at the source this should be a trivial change; is this something we could integrate for v4?

Thanks 👍

tmpfs added a commit to uwe-app/app that referenced this issue Oct 18, 2020
@sunng87
Copy link
Owner

sunng87 commented Oct 19, 2020

hi @tmpfs , because RenderContext is an internal mutable data to maintain the state during rendering. Exposing it in a render method is breaking my design. Could you please show me your code that produces stack overflows maybe I can offer some help.

@tmpfs
Copy link
Author

tmpfs commented Oct 20, 2020

Hi @sunng87 thanks for taking the time to look into this. Here is a trivial program that will create a stack overflow, it is annotated with some notes of other avenues I have explored for solving the problem:

use handlebars::*;

// NOTE: My program is multi-threaded and if I store the render stack
// NOTE: in an Arc<Mutex<_>> it will slow down the parallel processing
// NOTE: due to contention on the Mutex lock.

#[derive(Clone)]
pub struct Block {}

impl HelperDef for Block {
    fn call<'reg: 'rc, 'rc>(
        // NOTE: Cannot store the call stack on the helper
        // NOTE: because this is not &mut
        &self,
        _h: &Helper<'reg, 'rc>,
        r: &'reg Handlebars<'_>,
        ctx: &'rc Context,
        rc: &mut RenderContext<'reg, 'rc>,
        out: &mut dyn Output,
    ) -> HelperResult {
        // NOTE: In my real program this is a file path and i load
        // NOTE: the file content and split out the template from
        // NOTE: the front matter. It is this file path that i want
        // NOTE: to keep track of in a `stack`.

        let dynamic_template = rc
            .evaluate(ctx, "@root/template")?
            .as_json()
            .as_str()
            .ok_or_else(|| RenderError::new("Type error for `template`, string expected"))?
            .to_string();

        let result = r
            .render_template_with_context(&dynamic_template, ctx)
            .map_err(|_| RenderError::new(format!("Render error {}", dynamic_template)))?;

        out.write(&result)?;
        Ok(())
    }
}

fn main() {
    let mut h = Handlebars::new();
    h.register_helper("block", Box::new(Block {}));

    // Just invoke the helper in our template
    h.register_template_string("main", "{{block}}").unwrap();

    // NOTE: This is an example *naughty* dynamic partial template
    // NOTE: that will create a stack overflow by calling the `block`
    // NOTE: helper
    let bad_data = serde_json::json!({"template": "{{block}}"});

    h.render("main", &bad_data).unwrap();
}

From my point of view the logical place to store this private call stack information would be on the RenderContext but only if I can pass it into the render for the dynamic template.

Thanks for taking a look 👍

@tmpfs
Copy link
Author

tmpfs commented Oct 20, 2020

Furthermore while I was researching this I read that issue and noticed how you went about detecting stack overflows can easily be defeated with a single indirection. So I think we need to review how stack overflows are detected. Here is an example that will cause a stack overflow:

use handlebars::*;

fn main() {
    let mut h = Handlebars::new();
    h.register_partial("a", "{{> b}}").unwrap();
    h.register_partial("b", "{{> a}}").unwrap();
    h.register_template_string("main", "{{> a}}").unwrap();
    let data = serde_json::json!({});
    h.render("main", &data).unwrap();
}
thread 'main' has overflowed its stack
fatal runtime error: stack overflow
[1]    13987 abort      cargo run

But your stack overflow check will work correctly without the extra indirection:

use handlebars::*;

fn main() {
    let mut h = Handlebars::new();
    h.register_partial("a", "{{> a}}").unwrap();
    h.register_template_string("main", "{{> a}}").unwrap();
    let data = serde_json::json!({});
    h.render("main", &data).unwrap();
}

Yields:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: RenderError { desc: "Cannot include self in >", template_name: Some("a"), line_no: Some(1), column_no: Some(1), cause: None }', src/main.rs:8:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Happy to work with you to try to fix this if you like 👍

@sunng87
Copy link
Owner

sunng87 commented Oct 20, 2020

Hello @tmpfs

For your first example, I will suggest you to render the dynamic template outside the helper if possible:

let dyn_tpl = data.get("template").as_str().unwrap();
let intermediate_result = hbs.render_template(dyn_tpl, data).unwrap();
let result = hbs.render_template("{{block}}", json!({"block": intermediate_result}).unwrap();

For the second case, a fix is welcomed!

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

No branches or pull requests

2 participants