Fix unbounded recursion in Parser::parse_expression

[evanrichter]

fixes a stack overflow with malformed justfiles:

$ just -f <(python -c "print('A:(A' + '('*5000)")
thread 'main' has overflowed its stack
fatal runtime error: stack overflow
Aborted (core dumped)

[casey]

Nice! A few suggestions:

• This should have a test that makes sure that the right error is returned when parsing a deep expression. You can create a new file tests/recursion_limit.rs, and put the test in there. Check out other tests that use the Test struct for ideas.
• I think conceptually, it would be nicer to do this by starting with depth being 0, and increasing it whenever we recurse, and then checking if it's hit the limit instead of checking if it's reached zero.
• Is it necessary to save and restore the previous depth? I think it should be enough to increment it, recurse, and then decrement it when recurse returns.

[evanrichter]

• This should have a test that makes sure that the right error is returned when parsing a deep expression. You can create a new file tests/recursion_limit.rs, and put the test in there. Check out other tests that use the Test struct for ideas.
• ...starting with depth being 0, and increasing it

• It's ugly, but I got a test now :)
• Sure, that makes sense!

• Is it necessary to save and restore the previous depth? I think it should be enough to increment it, recurse, and then decrement it when recurse returns.

I did it that way more as a guard for programming errors. Since inside the body closure, self.depth can be modified (most likely would be a bug), and upon returning I thought I'd rather just restore it to what I know it should be. Now it's a simple inc/exec/dec and I tested some programming errors like adding or subtracting depth where you shouldn't, and the tests quickly blew up. So I think your way is better.

I also made the recursion depth u8::MAX for all compilation configurations. It makes cargo test agree with cargo test --release and I think 255 is plenty recursion even if release mode can technically handle more.

[casey]

This is definitely the best justfile in the test suite.

[casey]

Added a bunch of comments. It looks like the stack is being overrun on windows.

One issue is that we're actually increasing the size of the stack with this change, I think by two frames per expression. We'll have an extra frame for Parser::recurse, and another frame for the closure. This is probably what's causing the windows stack to overflow.

I think we should switch to just incrementing self.recursion_depth at the beginning of Parser::parse_expression, parsing the expression and saving it to a variable, and then decrementing it before returning. We don't even really need to handle the case in which parsing fails, because in that case parsing will stop. Als

[evanrichter]

I applied all the rewording suggestions, shortened the greatest test justfile by an order of magnitude, and inlined the recursion check.

Windows test is still failing with stack overflow :(

I'll try reducing the limit from 255 to 128

[evanrichter]

decreased again to 64 for windows

If this goes on much longer, maybe we should run tests in release mode?

[casey]

How about we just drop it down for windows:

Suggested change

	if self.depth == 64 {
	if self.depth == if cfg!(windows) { 64 } else { 256 } {

[evanrichter]

this change is in the newest push (though 255 not 256)

[casey]

There are some question marks in the parse_expression function which will bypass decrementing the recursion depth, which is fine, but let's make it consistent, i.e. we don't save a result, just the expression:

    let expression = 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()?);
        Expression::Concatenation { lhs, rhs }
      } else {
        value
      }
    };

    Ok(expression)

[casey]

Nice, merged! Thank you very much for finding and fixing this 🙇‍♂️