From f5eed7ae0a9b6baef3e9c3a08fbc40ee233999ef Mon Sep 17 00:00:00 2001
From: Tobias Guggenmos <tguggenm@redhat.com>
Date: Fri, 17 Jan 2020 14:06:27 +0100
Subject: [PATCH] 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      | 62 ++++++++++++++++++++++----------------------
 promql/parse_test.go |  4 +++
 2 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/promql/parse.go b/promql/parse.go
index 03494662d..fcacb04a4 100644
--- a/promql/parse.go
+++ b/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)
 		}
diff --git a/promql/parse_test.go b/promql/parse_test.go
index 9bd6e57df..7792a9b97 100644
--- a/promql/parse_test.go
+++ b/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.
 	{