From 06b50590a7f1e89694e5984edcecff9a9689b013 Mon Sep 17 00:00:00 2001 From: Goutham Veeramachaneni Date: Wed, 6 May 2020 14:51:39 +0200 Subject: [PATCH] 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 * Add tests for the panic Signed-off-by: Goutham Veeramachaneni --- pkg/logql/ast.go | 6 +++++- pkg/logql/ast_test.go | 22 ++++++++++++++++++++++ pkg/logql/filter.go | 19 +++++++++++++++---- pkg/logql/filter_test.go | 4 ++-- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/pkg/logql/ast.go b/pkg/logql/ast.go index ab71ec5164..4ad4444ac8 100644 --- a/pkg/logql/ast.go +++ b/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 } diff --git a/pkg/logql/ast_test.go b/pkg/logql/ast_test.go index c0ced73b99..e18887e2f0 100644 --- a/pkg/logql/ast_test.go +++ b/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 diff --git a/pkg/logql/filter.go b/pkg/logql/filter.go index d4a2e9760a..b9d77a903e 100644 --- a/pkg/logql/filter.go +++ b/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, diff --git a/pkg/logql/filter_test.go b/pkg/logql/filter_test.go index 89005eb2ce..bd6fd5748d 100644 --- a/pkg/logql/filter_test.go +++ b/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},