Fix bug in logql parsing that leads to crash. (#2046)

* Fix bug in logql parsing that leads to crash.

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x20ae356]
goroutine 39530021 [running]:
github.com/grafana/loki/pkg/logql.andFilter.Filter(0x0, 0x0, 0x3beffc0, 0xc01f065bc0, 0xc01aaaec00, 0x13d, 0x200, 0xffffffffffffffff)
	/src/loki/pkg/logql/filter.go:66 +0x26
github.com/grafana/loki/pkg/chunkenc.(*bufferedIterator).Next(0xc00a48fba0, 0x0)
	/src/loki/pkg/chunkenc/memchunk.go:616 +0x1cb
github.com/grafana/loki/pkg/iter.(*nonOverlappingIterator).Next(0xc00130ae40, 0x0)
	/src/loki/pkg/iter/iterator.go:438 +0x135
github.com/grafana/loki/pkg/iter.(*timeRangedIterator).Next(0xc00130ae80, 0x0)
	/src/loki/pkg/iter/iterator.go:497 +0x48
github.com/grafana/loki/pkg/iter.(*reverseIterator).load(0xc00072ac40)
	/src/loki/pkg/iter/iterator.go:555 +0x70
github.com/grafana/loki/pkg/iter.(*reverseIterator).Next(0xc00072ac40, 0x0)
	/src/loki/pkg/iter/iterator.go:563 +0x2f
github.com/grafana/loki/pkg/iter.(*nonOverlappingIterator).Next(0xc00130af40, 0x10)
	/src/loki/pkg/iter/iterator.go:438 +0x135
```

This is caused by people doing `|= ""` which would result in parsing of
TrueFilter but would be returned as nil.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Add tests for the panic

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

pkg/logql/ast.go

@@ -131,11 +131,15 @@ func (e *filterExpr) Filter() (LineFilter, error) {
 		if err != nil {
 			return nil, err
 		}
-		f = newAndFilter(nextFilter, f)
+		if nextFilter != nil {
+			f = newAndFilter(nextFilter, f)
+		}
 	}
+
 	if f == TrueFilter {
 		return nil, nil
 	}
+
 	return f, nil
 }
 

pkg/logql/ast_test.go

@@ -81,6 +81,28 @@ func Test_SampleExpr_String(t *testing.T) {
 	}
 }
 
+func Test_NilFilterDoesntPanic(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []string{
+		`{namespace="dev", container_name="cart"} |= "" |= "bloop"`,
+		`{namespace="dev", container_name="cart"} |= "bleep" |= ""`,
+		`{namespace="dev", container_name="cart"} |= "bleep" |= "" |= "bloop"`,
+		`{namespace="dev", container_name="cart"} |= "bleep" |= "" |= "bloop"`,
+		`{namespace="dev", container_name="cart"} |= "bleep" |= "bloop" |= ""`,
+	} {
+		t.Run(tc, func(t *testing.T) {
+			expr, err := ParseLogSelector(tc)
+			require.Nil(t, err)
+
+			filter, err := expr.Filter()
+			require.Nil(t, err)
+
+			require.True(t, filter.Filter([]byte("bleepbloop")))
+		})
+	}
+
+}
+
 type linecheck struct {
 	l string
 	e bool

pkg/logql/filter.go

@@ -53,9 +53,15 @@ type andFilter struct {
 
 // newAndFilter creates a new filter which matches only if left and right matches.
 func newAndFilter(left LineFilter, right LineFilter) LineFilter {
-	if (right == TrueFilter || right == nil) && (left == TrueFilter || left == nil) {
-		return TrueFilter
+	// Make sure we take care of panics in case a nil or noop filter is passed.
+	if right == nil || right == TrueFilter {
+		return left
+	}
+
+	if left == nil || left == TrueFilter {
+		return right
 	}
+
 	return andFilter{
 		left:  left,
 		right: right,
@@ -73,9 +79,14 @@ type orFilter struct {
 
 // newOrFilter creates a new filter which matches only if left or right matches.
 func newOrFilter(left LineFilter, right LineFilter) LineFilter {
-	if (right == TrueFilter || right == nil) && (left == TrueFilter || left == nil) {
-		return TrueFilter
+	if left == nil || left == TrueFilter {
+		return right
+	}
+
+	if right == nil || right == TrueFilter {
+		return left
 	}
+
 	return orFilter{
 		left:  left,
 		right: right,

pkg/logql/filter_test.go

@@ -109,8 +109,8 @@ func Test_TrueFilter(t *testing.T) {
 		{"nil left or", newOrFilter(nil, newContainsFilter(empty, false)), true},
 		{"nil right and not empty", newAndFilter(newContainsFilter([]byte("foo"), false), nil), false},
 		{"nil left or not empty", newOrFilter(nil, newContainsFilter([]byte("foo"), false)), false},
-		{"nil both and", newAndFilter(nil, nil), true},
-		{"nil both or", newOrFilter(nil, nil), true},
+		{"nil both and", newAndFilter(nil, nil), false}, // returns nil
+		{"nil both or", newOrFilter(nil, nil), false},   // returns nil
 		{"empty match and chained", newAndFilter(newContainsFilter(empty, false), newAndFilter(newContainsFilter(empty, false), newAndFilter(newContainsFilter(empty, false), newContainsFilter(empty, false)))), true},
 		{"empty match or chained", newOrFilter(newContainsFilter(empty, false), newOrFilter(newContainsFilter(empty, true), newOrFilter(newContainsFilter(empty, false), newContainsFilter(empty, false)))), true},
 		{"empty match and", newNotFilter(newAndFilter(newContainsFilter(empty, false), newContainsFilter(empty, false))), false},