From ec5b8ce34798e5af45168f5c79629285a92c2f3a Mon Sep 17 00:00:00 2001 From: Evan Richter Date: Mon, 20 Jun 2022 21:34:02 -0500 Subject: [PATCH 1/2] Fix unbounded recursion in Parser::parse_expression --- src/compile_error.rs | 3 +++ src/compile_error_kind.rs | 1 + src/parser.rs | 48 ++++++++++++++++++++++++++++++--------- tests/lib.rs | 1 + tests/recursion_limit.rs | 21 +++++++++++++++++ 5 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 tests/recursion_limit.rs diff --git a/src/compile_error.rs b/src/compile_error.rs index ba8d09eb84..c772fa0d9b 100644 --- a/src/compile_error.rs +++ b/src/compile_error.rs @@ -198,6 +198,9 @@ impl Display for CompileError<'_> { parameter )?; } + RecursionDepthExceeded => { + write!(f, "Recursion depth was exceeded")?; + } RequiredParameterFollowsDefaultParameter { parameter } => { write!( f, diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs index 062659bac2..9661a1faa6 100644 --- a/src/compile_error_kind.rs +++ b/src/compile_error_kind.rs @@ -74,6 +74,7 @@ pub(crate) enum CompileErrorKind<'src> { ParameterShadowsVariable { parameter: &'src str, }, + RecursionDepthExceeded, RequiredParameterFollowsDefaultParameter { parameter: &'src str, }, diff --git a/src/parser.rs b/src/parser.rs index 9f77de4c97..3aad3d3936 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -32,9 +32,13 @@ pub(crate) struct Parser<'tokens, 'src> { next: usize, /// Current expected tokens expected: BTreeSet, + /// Current recursion depth + depth: u8, } impl<'tokens, 'src> Parser<'tokens, 'src> { + const RECURSION_LIMIT: u8 = u8::MAX; + /// Parse `tokens` into an `Ast` pub(crate) fn parse(tokens: &'tokens [Token<'src>]) -> CompileResult<'src, Ast<'src>> { Self::new(tokens).parse_ast() @@ -46,6 +50,7 @@ impl<'tokens, 'src> Parser<'tokens, 'src> { next: 0, expected: BTreeSet::new(), tokens, + depth: 0, } } @@ -389,21 +394,42 @@ impl<'tokens, 'src> Parser<'tokens, 'src> { }) } + /// Bound recursion to self.depth + fn recurse(&mut self, body: F) -> CompileResult<'src, Expression<'src>> + where + F: Fn(&mut Self) -> CompileResult<'src, Expression<'src>>, + { + if self.depth == Self::RECURSION_LIMIT { + return Err(CompileError { + token: self.next()?, + kind: CompileErrorKind::RecursionDepthExceeded, + }); + } + + self.depth += 1; + let result = body(self); + self.depth -= 1; + + result + } + /// Parse an expression, e.g. `1 + 2` fn parse_expression(&mut self) -> CompileResult<'src, Expression<'src>> { - if self.accepted_keyword(Keyword::If)? { - self.parse_conditional() - } else { - let value = self.parse_value()?; - - if self.accepted(Plus)? { - let lhs = Box::new(value); - let rhs = Box::new(self.parse_expression()?); - Ok(Expression::Concatenation { lhs, rhs }) + self.recurse(|slf| { + if slf.accepted_keyword(Keyword::If)? { + slf.parse_conditional() } else { - Ok(value) + let value = slf.parse_value()?; + + if slf.accepted(Plus)? { + let lhs = Box::new(value); + let rhs = Box::new(slf.parse_expression()?); + Ok(Expression::Concatenation { lhs, rhs }) + } else { + Ok(value) + } } - } + }) } /// Parse a conditional, e.g. `if a == b { "foo" } else { "bar" }` diff --git a/tests/lib.rs b/tests/lib.rs index 0827bba5e9..671450b7ce 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -62,6 +62,7 @@ mod positional_arguments; mod quiet; mod quote; mod readme; +mod recursion_limit; mod regexes; mod run; mod search; diff --git a/tests/recursion_limit.rs b/tests/recursion_limit.rs new file mode 100644 index 0000000000..469e8193d4 --- /dev/null +++ b/tests/recursion_limit.rs @@ -0,0 +1,21 @@ +use super::*; + +#[test] +fn bugfix() { + let mut justfile = String::from("foo: (x "); + for _ in 0..5000 { + justfile.push('('); + } + Test::new() + .justfile(&justfile) + .stderr(RECURSION_LIMIT_REACHED) + .status(EXIT_FAILURE) + .run(); +} + +const RECURSION_LIMIT_REACHED: &str = " +error: Recursion depth was exceeded + | +1 | foo: (x (((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((( + | ^ +"; From ee7cb18b26d34a9c443e942ca9f4b15fdc64efe3 Mon Sep 17 00:00:00 2001 From: Evan Richter Date: Wed, 22 Jun 2022 09:13:36 -0500 Subject: [PATCH 2/2] recursion depth checks clean up * inline recursion check * rename recursion depth error * reword recursion error message * shorten recursion test * windows recursion is capped at 64, other platforms at 255 --- src/compile_error.rs | 4 ++-- src/compile_error_kind.rs | 2 +- src/parser.rs | 45 +++++++++++++++------------------------ tests/recursion_limit.rs | 15 ++++++++++--- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/compile_error.rs b/src/compile_error.rs index c772fa0d9b..b4def32413 100644 --- a/src/compile_error.rs +++ b/src/compile_error.rs @@ -198,8 +198,8 @@ impl Display for CompileError<'_> { parameter )?; } - RecursionDepthExceeded => { - write!(f, "Recursion depth was exceeded")?; + ParsingRecursionDepthExceeded => { + write!(f, "Parsing recursion depth exceeded")?; } RequiredParameterFollowsDefaultParameter { parameter } => { write!( diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs index 9661a1faa6..a425e24787 100644 --- a/src/compile_error_kind.rs +++ b/src/compile_error_kind.rs @@ -74,7 +74,7 @@ pub(crate) enum CompileErrorKind<'src> { ParameterShadowsVariable { parameter: &'src str, }, - RecursionDepthExceeded, + ParsingRecursionDepthExceeded, RequiredParameterFollowsDefaultParameter { parameter: &'src str, }, diff --git a/src/parser.rs b/src/parser.rs index 3aad3d3936..fd42452092 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -37,8 +37,6 @@ pub(crate) struct Parser<'tokens, 'src> { } impl<'tokens, 'src> Parser<'tokens, 'src> { - const RECURSION_LIMIT: u8 = u8::MAX; - /// Parse `tokens` into an `Ast` pub(crate) fn parse(tokens: &'tokens [Token<'src>]) -> CompileResult<'src, Ast<'src>> { Self::new(tokens).parse_ast() @@ -394,42 +392,33 @@ impl<'tokens, 'src> Parser<'tokens, 'src> { }) } - /// Bound recursion to self.depth - fn recurse(&mut self, body: F) -> CompileResult<'src, Expression<'src>> - where - F: Fn(&mut Self) -> CompileResult<'src, Expression<'src>>, - { - if self.depth == Self::RECURSION_LIMIT { + /// Parse an expression, e.g. `1 + 2` + fn parse_expression(&mut self) -> CompileResult<'src, Expression<'src>> { + if self.depth == if cfg!(windows) { 64 } else { 255 } { return Err(CompileError { token: self.next()?, - kind: CompileErrorKind::RecursionDepthExceeded, + kind: CompileErrorKind::ParsingRecursionDepthExceeded, }); } self.depth += 1; - let result = body(self); - self.depth -= 1; - result - } + let expression = if self.accepted_keyword(Keyword::If)? { + self.parse_conditional()? + } else { + let value = self.parse_value()?; - /// Parse an expression, e.g. `1 + 2` - fn parse_expression(&mut self) -> CompileResult<'src, Expression<'src>> { - self.recurse(|slf| { - if slf.accepted_keyword(Keyword::If)? { - slf.parse_conditional() + if self.accepted(Plus)? { + let lhs = Box::new(value); + let rhs = Box::new(self.parse_expression()?); + Expression::Concatenation { lhs, rhs } } else { - let value = slf.parse_value()?; - - if slf.accepted(Plus)? { - let lhs = Box::new(value); - let rhs = Box::new(slf.parse_expression()?); - Ok(Expression::Concatenation { lhs, rhs }) - } else { - Ok(value) - } + value } - }) + }; + + self.depth -= 1; + Ok(expression) } /// Parse a conditional, e.g. `if a == b { "foo" } else { "bar" }` diff --git a/tests/recursion_limit.rs b/tests/recursion_limit.rs index 469e8193d4..40782c35eb 100644 --- a/tests/recursion_limit.rs +++ b/tests/recursion_limit.rs @@ -3,7 +3,7 @@ use super::*; #[test] fn bugfix() { let mut justfile = String::from("foo: (x "); - for _ in 0..5000 { + for _ in 0..500 { justfile.push('('); } Test::new() @@ -13,9 +13,18 @@ fn bugfix() { .run(); } +#[cfg(not(windows))] const RECURSION_LIMIT_REACHED: &str = " -error: Recursion depth was exceeded +error: Parsing recursion depth exceeded | -1 | foo: (x (((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((( +1 | foo: (x (((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((( | ^ "; + +#[cfg(windows)] +const RECURSION_LIMIT_REACHED: &str = " +error: Parsing recursion depth exceeded + | +1 | foo: (x (((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((( + | ^ +";