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

Fix stack smashing by converting recursion to iteration #63

Merged
merged 5 commits into from
May 3, 2018

Conversation

brson
Copy link
Collaborator

@brson brson commented May 3, 2018

It's possible to feed comrak input that causes it to run off the end of the stack, e.g., a long series of ">>>>" is a nested blockquote and will recurse deeply.

This eliminates all the recursion I could find.

Note that the change to postprocess_text_nodes actually changes the order nodes are visited, and in a way that could affect correctness and cache behavior. I benchmarked the change and couldn't see any conclusive difference in perf.

brson added 2 commits May 3, 2018 13:04
Note that this changes the traversal order slightly, processing
all siblings before descending into children. This could affect
cache behavior in subsequent passes, but looking at the benchmark
I haven't found anything conclusive.
@brson brson changed the title Fix stack smashing be converting recursion to iteration Fix stack smashing by converting recursion to iteration May 3, 2018
@kivikakk
Copy link
Owner

kivikakk commented May 3, 2018

So good! ❤️ Thank you!

@kivikakk kivikakk merged commit 26c1dba into kivikakk:master May 3, 2018
@kivikakk
Copy link
Owner

kivikakk commented May 3, 2018

btw, I figure this won't be super relevant since you're working off a fork, but a crate release would ever be helpful to y'all, please feel free to shout.

@brson
Copy link
Collaborator Author

brson commented May 4, 2018

Thanks. It's probably worth putting this out as a point release so that crates.io and docs.rs can get it.

@kivikakk
Copy link
Owner

kivikakk commented May 4, 2018

👍

brson pushed a commit to brson/comrak that referenced this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants