fix(parser): apply recursion limit everywhere, reduce default to 500 (#…

…662)

The limit was only tracked for nested selection sets, but the parser turns out
to use recursion in other cases too. Issue #666 tracks reducing them.
Stack overflow was observed with little more than 2000
nesting levels or repetitions in the new test.
Defaulting to a quarter of that leaves a comfortable margin.

crates/apollo-compiler/src/database/ast.rs

@@ -190,7 +190,10 @@ mod tests {
         assert_eq!(ast.errors().len(), 1);
         assert_eq!(
             ast.errors().next(),
-            Some(&apollo_parser::Error::limit("parser limit(3) reached", 121))
+            Some(&apollo_parser::Error::limit(
+                "parser recursion limit reached",
+                121
+            ))
         );
     }
 

crates/apollo-compiler/test_data/ok/0001_annonymous_operation_definition.txt

@@ -61,4 +61,4 @@
                         - [email protected] "String"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0002_multiple_named_operation_definitions.txt

@@ -128,4 +128,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 3
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0003_schema_definition_with_custom_operation_types.txt

@@ -173,4 +173,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0004_schema_with_custom_scalars.txt

@@ -50,4 +50,4 @@
                         - [email protected]
                             - [email protected] "\"https://tools.ietf.org/html/rfc3986\""
                     - [email protected] ")"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0005_schema_with_valid_enum_definitions.txt

@@ -81,4 +81,4 @@
                         - [email protected] "ACANA"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0006_schema_with_valid_union.txt

@@ -111,4 +111,4 @@
                         - [email protected] "SearchResult"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0007_schema_with_interface_definition.txt

@@ -158,4 +158,4 @@
                         - [email protected] "String"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0008_schema_with_directive_definition.txt

@@ -104,4 +104,4 @@
                     - [email protected] "!"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0009_schema_with_input_object.txt

@@ -79,4 +79,4 @@
                         - [email protected] "Float"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0010_operation_with_defined_fields.txt

@@ -218,4 +218,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0011_fragment_spreads_in_fragment_definitions.txt

@@ -143,4 +143,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0012_introspection_query.txt

@@ -477,4 +477,4 @@
                     - [email protected] "}"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 8
+recursion limit: 500, high: 8

crates/apollo-compiler/test_data/ok/0013_operation_with_used_variable_in_fragment.txt

@@ -229,4 +229,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0014_float_values.txt

@@ -115,4 +115,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 4

crates/apollo-compiler/test_data/ok/0015_supergraph.txt

@@ -4200,4 +4200,4 @@
                         - [email protected] "String"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 7

crates/apollo-compiler/test_data/ok/0016_same_variables_in_multiple_operations.txt

@@ -186,4 +186,4 @@
                     - [email protected] "!"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0017_variables_are_input_types.txt

@@ -300,4 +300,4 @@
                         - [email protected] "Boolean"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0018_non_clashing_names.txt

@@ -110,4 +110,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0019_extensions.txt

@@ -225,4 +225,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0020_merge_identical_fields.txt

@@ -147,4 +147,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0021_merge_identical_fields_with_arguments.txt

@@ -238,4 +238,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0022_merge_differing_fields_and_args.txt

@@ -541,4 +541,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0024_used_variables_in_directives.txt

@@ -281,4 +281,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 1
+recursion limit: 500, high: 5

crates/apollo-compiler/test_data/ok/0025_unique_directives.txt

@@ -88,4 +88,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 1
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0026_type_introspection.txt

@@ -113,4 +113,4 @@
                     - [email protected] "}"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 4
+recursion limit: 500, high: 4

crates/apollo-compiler/test_data/ok/0027_typename_introspection_in_object.txt

@@ -72,4 +72,4 @@
                     - [email protected] "__typename"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 1
+recursion limit: 500, high: 1

crates/apollo-compiler/test_data/ok/0028_typename_introspection_in_union.txt

@@ -152,4 +152,4 @@
                     - [email protected] "}"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0029_used_variable_in_list_and_input.txt

@@ -214,4 +214,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 4

crates/apollo-compiler/test_data/ok/0030_cyclical_nullable_input_objects.txt

@@ -152,4 +152,4 @@
                         - [email protected] "First"
             - [email protected] "\n"
             - [email protected] "}"
-recursion limit: 4096, high: 0
+recursion limit: 500, high: 2

crates/apollo-compiler/test_data/ok/0031_fragment_spread_possible.txt

@@ -712,4 +712,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 3
+recursion limit: 500, high: 3

crates/apollo-compiler/test_data/ok/0032_valid_of_correct_type.txt

@@ -2808,4 +2808,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n"
-recursion limit: 4096, high: 3
+recursion limit: 500, high: 8

crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.txt

@@ -224,4 +224,4 @@
             - [email protected] "\n"
             - [email protected] "}"
     - [email protected] "\n\n"
-recursion limit: 4096, high: 2
+recursion limit: 500, high: 4

crates/apollo-parser/CHANGELOG.md

@@ -19,11 +19,19 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 # [x.x.x] (unreleased) - 2023-mm-dd
 ## Fixes
+- **apply recursion limit where needed, reduce its default from 4096 to 500 [SimonSapin], [pull/662]**
+  The limit was only tracked for nested selection sets, but the parser turns out
+  to use recursion in other cases too. [Issue 666] tracks reducing them.
+  Stack overflow was observed with little more than 2000
+  nesting levels or repetitions in the new test.
+  Defaulting to a quarter of that leaves a comfortable margin.
 - **fixes lexing of plus and minus signs in numbers - [SimonSapin], [pull/646]**
   Plus signs are errors in GraphQL syntax.
   Minus signs are errors if they’re not followed by a digit, even if followed by EOF.
 
 [pull/646]: https://github.com/apollographql/apollo-rs/pull/646
+[pull/662]: https://github.com/apollographql/apollo-rs/pull/662
+[Issue 666]: https://github.com/apollographql/apollo-rs/issues/666
 
 # [0.6.2](https://crates.io/crates/apollo-parser/0.6.2) - 2023-09-08
 ## Fixes

crates/apollo-parser/src/lexer/mod.rs

@@ -107,8 +107,7 @@ impl<'a> Iterator for Lexer<'a> {
             return None;
         }
 
-        self.limit_tracker.consume();
-        if self.limit_tracker.limited() {
+        if self.limit_tracker.check_and_increment() {
             self.finished = true;
             return Some(Err(Error::limit(
                 "token limit reached, aborting lexing",

crates/apollo-parser/src/limit.rs

@@ -39,23 +39,13 @@ use std::fmt;
 /// ```
 #[derive(PartialEq, Eq, Clone, Copy)]
 pub struct LimitTracker {
-    current: usize,
+    pub(crate) current: usize,
     /// High Water mark for this limit
     pub high: usize,
     /// Limit.
     pub limit: usize,
 }
 
-impl Default for LimitTracker {
-    fn default() -> Self {
-        Self {
-            current: 0,
-            high: 0,
-            limit: 4_096, // Recursion limit derived from router experimentation
-        }
-    }
-}
-
 impl LimitTracker {
     pub fn new(limit: usize) -> Self {
         Self {
@@ -65,19 +55,23 @@ impl LimitTracker {
         }
     }
 
-    pub fn limited(&self) -> bool {
-        self.current > self.limit
-    }
-
-    pub fn consume(&mut self) {
+    /// Return whether the limit was reached
+    #[must_use]
+    pub fn check_and_increment(&mut self) -> bool {
         self.current += 1;
         if self.current > self.high {
             self.high = self.current;
         }
+        let reached = self.current > self.limit;
+        if reached {
+            // Caller is gonna return early, keep increments and decrements balanced:
+            self.decrement()
+        }
+        reached
     }
 
-    pub fn reset(&mut self) {
-        self.current = 0;
+    pub fn decrement(&mut self) {
+        self.current -= 1;
     }
 }
 

crates/apollo-parser/src/parser/grammar/argument.rs

@@ -17,7 +17,14 @@ pub(crate) fn argument(p: &mut Parser, mut is_argument: bool) {
             is_argument = true;
             if p.peek().is_some() {
                 guard.finish_node();
-                return argument(p, is_argument);
+                // TODO: use a loop instead of recursion
+                if p.recursion_limit.check_and_increment() {
+                    p.limit_err("parser recursion limit reached");
+                    return;
+                }
+                argument(p, is_argument);
+                p.recursion_limit.decrement();
+                return;
             }
         }
     }

crates/apollo-parser/src/parser/grammar/document.rs

@@ -14,9 +14,10 @@ pub(crate) fn document(p: &mut Parser) {
     let doc = p.start_node(SyntaxKind::DOCUMENT);
 
     while let Some(node) = p.peek() {
-        if p.recursion_limit.limited() {
-            break;
-        }
+        assert_eq!(
+            p.recursion_limit.current, 0,
+            "unbalanced limit increment / decrement"
+        );
 
         match node {
             TokenKind::StringValue => {

crates/apollo-parser/src/parser/grammar/enum_.rs

@@ -101,7 +101,14 @@ pub(crate) fn enum_value_definition(p: &mut Parser) {
         }
         if p.peek().is_some() {
             guard.finish_node();
-            return enum_value_definition(p);
+            // TODO: use a loop instead of recursion
+            if p.recursion_limit.check_and_increment() {
+                p.limit_err("parser recursion limit reached");
+                return;
+            }
+            enum_value_definition(p);
+            p.recursion_limit.decrement();
+            return;
         }
     }
 

crates/apollo-parser/src/parser/grammar/fragment.rs

@@ -19,7 +19,7 @@ pub(crate) fn fragment_definition(p: &mut Parser) {
     }
 
     match p.peek() {
-        Some(T!['{']) => selection::top_selection_set(p),
+        Some(T!['{']) => selection::selection_set(p),
         _ => p.err("expected a Selection Set"),
     }
 }
@@ -77,7 +77,7 @@ pub(crate) fn inline_fragment(p: &mut Parser) {
     }
 
     match p.peek() {
-        Some(T!['{']) => selection::top_selection_set(p),
+        Some(T!['{']) => selection::selection_set(p),
         _ => p.err("expected Selection Set"),
     }
 }

crates/apollo-parser/src/parser/grammar/input.rs

@@ -105,7 +105,14 @@ pub(crate) fn input_value_definition(p: &mut Parser, is_input: bool) {
 
                     if p.peek().is_some() {
                         guard.finish_node();
-                        return input_value_definition(p, true);
+                        // TODO: use a loop instead of recursion
+                        if p.recursion_limit.check_and_increment() {
+                            p.limit_err("parser recursion limit reached");
+                            return;
+                        }
+                        input_value_definition(p, true);
+                        p.recursion_limit.decrement();
+                        return;
                     }
                 }
                 _ => p.err("expected a Type"),

crates/apollo-parser/src/parser/grammar/object.rs

@@ -104,7 +104,13 @@ fn implements_interface(p: &mut Parser, is_interfaces: bool) {
             ty::named_type(p);
             if let Some(node) = p.peek_data() {
                 if !is_definition(node) {
+                    // TODO: use a loop instead of recursion
+                    if p.recursion_limit.check_and_increment() {
+                        p.limit_err("parser recursion limit reached");
+                        return;
+                    }
                     implements_interface(p, true);
+                    p.recursion_limit.decrement()
                 }
 
                 return;