PromQL: Fix panic in parser (#6650)

Fixes #6649.

The crash is fixed here, was caused because some  AST sanity checks were performed on the syntax tree while assembling it. In case of previous parsing errors this could lead to undefined behaviour.

The fix is to move the checks to the typechecking phase, which runs only when a syntax tree was assembled without there being parsing errors.

There are other places, where similiar checks are performed while assembling the syntax tree. It might be a good idea to move those to the typechecking phase, too. Should I do this in the same or a separate PR?

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>

promql/parse.go

@@ -350,37 +350,6 @@ func (p *parser) newBinaryExpression(lhs Node, op Item, modifiers Node, rhs Node
 	ret.RHS = rhs.(Expr)
 	ret.Op = op.Typ
 
-	// opRange returns the PositionRange of the operator part of the BinaryExpr.
-	// This is made a function instead of a variable, so it is lazily evaluated on demand.
-	opRange := func() (r PositionRange) {
-		// Remove whitespace at the beginning and end of the range.
-		for r.Start = lhs.PositionRange().End; isSpace(rune(p.lex.input[r.Start])); r.Start++ {
-		}
-		for r.End = rhs.PositionRange().Start - 1; isSpace(rune(p.lex.input[r.End])); r.End-- {
-		}
-		return
-	}
-
-	if ret.ReturnBool && !op.Typ.isComparisonOperator() {
-		p.failf(opRange(), "bool modifier can only be used on comparison operators")
-	}
-
-	if op.Typ.isComparisonOperator() && !ret.ReturnBool && ret.RHS.Type() == ValueTypeScalar && ret.LHS.Type() == ValueTypeScalar {
-		p.failf(opRange(), "comparisons between scalars must use BOOL modifier")
-	}
-
-	if op.Typ.isSetOperator() && ret.VectorMatching.Card == CardOneToOne {
-		ret.VectorMatching.Card = CardManyToMany
-	}
-
-	for _, l1 := range ret.VectorMatching.MatchingLabels {
-		for _, l2 := range ret.VectorMatching.Include {
-			if l1 == l2 && ret.VectorMatching.On {
-				p.failf(opRange(), "label %q must not occur in ON and GROUP clause at once", l1)
-			}
-		}
-	}
-
 	return ret
 }
 
@@ -524,6 +493,37 @@ func (p *parser) checkType(node Node) (typ ValueType) {
 		lt := p.checkType(n.LHS)
 		rt := p.checkType(n.RHS)
 
+		// opRange returns the PositionRange of the operator part of the BinaryExpr.
+		// This is made a function instead of a variable, so it is lazily evaluated on demand.
+		opRange := func() (r PositionRange) {
+			// Remove whitespace at the beginning and end of the range.
+			for r.Start = n.LHS.PositionRange().End; isSpace(rune(p.lex.input[r.Start])); r.Start++ {
+			}
+			for r.End = n.RHS.PositionRange().Start - 1; isSpace(rune(p.lex.input[r.End])); r.End-- {
+			}
+			return
+		}
+
+		if n.ReturnBool && !n.Op.isComparisonOperator() {
+			p.failf(opRange(), "bool modifier can only be used on comparison operators")
+		}
+
+		if n.Op.isComparisonOperator() && !n.ReturnBool && n.RHS.Type() == ValueTypeScalar && n.LHS.Type() == ValueTypeScalar {
+			p.failf(opRange(), "comparisons between scalars must use BOOL modifier")
+		}
+
+		if n.Op.isSetOperator() && n.VectorMatching.Card == CardOneToOne {
+			n.VectorMatching.Card = CardManyToMany
+		}
+
+		for _, l1 := range n.VectorMatching.MatchingLabels {
+			for _, l2 := range n.VectorMatching.Include {
+				if l1 == l2 && n.VectorMatching.On {
+					p.failf(opRange(), "label %q must not occur in ON and GROUP clause at once", l1)
+				}
+			}
+		}
+
 		if !n.Op.isOperator() {
 			p.failf(n.PositionRange(), "binary expression does not support operator %q", n.Op)
 		}

promql/parse_test.go

@@ -2121,6 +2121,10 @@ var testExpr = []struct {
 		input:  "e-+=/(0)",
 		fail:   true,
 		errMsg: `unexpected "="`,
+	}, {
+		input:  "a>b()",
+		fail:   true,
+		errMsg: `unknown function`,
 	},
 	// String quoting and escape sequence interpretation tests.
 	{