Avoid stack overflows via configurable with_recursion_limit
#764
Conversation
| @@ -55,6 +56,92 @@ macro_rules! return_ok_if_some { | |||
| }}; | |||
| } | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| /// Implemenation [`RecursionCounter`] if std is available | |||
There was a problem hiding this comment.
This is the key change for actually limiting recursion depth
| /// each call to `try_decrease()`, when it reaches 0 an error will | ||
| /// be returned. | ||
| /// | ||
| /// Note: Uses an Rc and AtomicUsize in order to satisfy the Rust |
There was a problem hiding this comment.
The Rc approach is from @46bit -- if anyone has ideas about how to avoid it, I would love a PR to help.
| } | ||
| } | ||
|
|
||
| /// Guard that increass the remaining depth by 1 on drop |
There was a problem hiding this comment.
The DepthGuard is used to automatically ensure the recursion depth is restored upon function return
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn new(dialect: &'a dyn Dialect) -> Self { |
There was a problem hiding this comment.
the signature here has changed - I tried to illustrate the intended usage with doc comments
| } | ||
|
|
||
| /// Parse a SQL statement and produce an Abstract Syntax Tree (AST) | ||
| pub fn parse_sql(dialect: &dyn Dialect, sql: &str) -> Result<Vec<Statement>, ParserError> { |
There was a problem hiding this comment.
github mangles the diff -- this function still exists with the same signature. It now also has docstrings
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn parse_statements(&mut self) -> Result<Vec<Statement>, ParserError> { |
There was a problem hiding this comment.
This code was factored out of parse_sql so that people directly using Parser::new() could also have access to that logic
Pull Request Test Coverage Report for Build 3698164587
💛 - Coveralls |
|
cc @AugustoFKL @andygrove and @Dandandan |
alamb
changed the title
with_recursion_limit configurable recursion limit to parserwith_recursion_limit
Closes #305. Alternative to #751
This PR is based off the approach from @46bit in #501 and includes some code and tests from there. While it may look like a large change, it is mostly docstring changes and tests. The code changes are very small
Rationale
Given a deeply nested input,
sqlparserwill overflow the stack, resulting in a panic/segfault.Changes
parse_expr,parse_statementandparse_queryAPI Changes
The
Parser::new()signature is change, but it is now documented. All other changes are to unreleased code added in #710 by @ankrgylNotes
The only recursive nested structures are
Expr,Statement, andQuery(as they are the places that areBoxed in the AST). Thus it is sufficient to check recursion depth at each of these points