Skip to content

Avoid running out of stack space by limiting recursion depth in Parse(...) #5

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

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Avoid running out of stack space by limiting recursion depth in Parse(...) #5

merged 1 commit into from
Sep 12, 2019

Conversation

practicalswift
Copy link
Contributor

Avoid running out of stack space by limiting recursion depth in Parse(...).

Before:

$ python -c 'print(1000 * ":");' | ./miniscript |& grep -E '^(Failed|SUMMARY)'
SUMMARY: AddressSanitizer: stack-overflow miniscript/bitcoin/script/miniscript.h:752 in std::shared_ptr<miniscript::Node<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const> miniscript::internal::Parse<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, CompilerContext>(Span<char const>&, CompilerContext const&)

After:

$ python -c 'print(1000 * ":");' | ./miniscript |& grep -E '^(Failed|SUMMARY)'
Failed to parse as policy or miniscript '::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::'

There are obviously more than one way to solve this: let me know if another route is preferred.

Will the spec say something that could justify a limitation here?

@practicalswift
Copy link
Contributor Author

practicalswift commented Sep 4, 2019

Somewhat similar issue in the miniscript implementation in Rust: https://github.com/apoelstra/rust-miniscript/issues/41

@sipa
Copy link
Owner

sipa commented Sep 4, 2019

@practicalswift Nice find. It seems that it's impossible to create a valid Miniscript with a nesting depth higher than 402 (any such script will trivially exceed the ops limit of 201), so that seems like a natural limit to pick.

Now, those 402 consist of 201 v: wrappers and 201 other nodes. The Parse functions don't use recursion for wrappers, so really the recursion limit can be 201 for those.

We also need this kind of protection when converting from a Script to a Miniscript, as a similar recursive parser function is used there. For those we probably actually need a limit of 402.

@practicalswift
Copy link
Contributor Author

Thanks for clarifying! I'll implement.

@practicalswift
Copy link
Contributor Author

@sipa Would you mind re-reviewing the updated version? I'll handle the FromScript recursion limits in another PR.

@sipa
Copy link
Owner

sipa commented Sep 12, 2019

ACK

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