From 0ec041d3bca2acb379bfc8a4345c68fe5fd3502a Mon Sep 17 00:00:00 2001
From: Matt Tracy <matt@cockroachlabs.com>
Date: Mon, 12 Mar 2018 18:28:50 -0400
Subject: [PATCH] ts: Fix nil pointer panic on invalid TS query

A query which specified an invalid enumeration value for
SourceAggregator would cause a nil pointer panic on the server.
Repaired this fix and added a test for invalid values of all the
enumeration values of the query request.

Fixes #23766

Release note: None
---
 pkg/ts/query.go      | 10 ++++----
 pkg/ts/query_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/pkg/ts/query.go b/pkg/ts/query.go
index e18da747b264..eb929af514aa 100644
--- a/pkg/ts/query.go
+++ b/pkg/ts/query.go
@@ -1047,6 +1047,10 @@ func (db *DB) Query(
 		aggFn = aggregatingIterator.max
 	case tspb.TimeSeriesQueryAggregator_MIN:
 		aggFn = aggregatingIterator.min
+	default:
+		return nil, nil, fmt.Errorf(
+			"query specified unknown time series aggregator: %s", query.GetSourceAggregator().String(),
+		)
 	}
 
 	// Filter the result of the aggregation function through a leading edge
@@ -1124,7 +1128,6 @@ func makeDataSpans(
 	return sourceSpans, nil
 }
 
-// getExtractionFunction returns
 func getExtractionFunction(agg tspb.TimeSeriesQueryAggregator) (extractFn, error) {
 	switch agg {
 	case tspb.TimeSeriesQueryAggregator_AVG:
@@ -1136,7 +1139,7 @@ func getExtractionFunction(agg tspb.TimeSeriesQueryAggregator) (extractFn, error
 	case tspb.TimeSeriesQueryAggregator_MIN:
 		return (roachpb.InternalTimeSeriesSample).Minimum, nil
 	}
-	return nil, errors.Errorf("query specified unknown time series aggregator %s", agg.String())
+	return nil, errors.Errorf("query specified unknown time series downsampler %s", agg.String())
 }
 
 func downsampleSum(points ...roachpb.InternalTimeSeriesSample) float64 {
@@ -1177,7 +1180,6 @@ func downsampleAvg(points ...roachpb.InternalTimeSeriesSample) float64 {
 	return total / float64(count)
 }
 
-// getDownsampleFunction returns
 func getDownsampleFunction(agg tspb.TimeSeriesQueryAggregator) (downsampleFn, error) {
 	switch agg {
 	case tspb.TimeSeriesQueryAggregator_AVG:
@@ -1189,5 +1191,5 @@ func getDownsampleFunction(agg tspb.TimeSeriesQueryAggregator) (downsampleFn, er
 	case tspb.TimeSeriesQueryAggregator_MIN:
 		return downsampleMin, nil
 	}
-	return nil, errors.Errorf("query specified unknown time series aggregator %s", agg.String())
+	return nil, errors.Errorf("query specified unknown time series downsampler %s", agg.String())
 }
diff --git a/pkg/ts/query_test.go b/pkg/ts/query_test.go
index 1ac908842a1e..6cd869c38aa2 100644
--- a/pkg/ts/query_test.go
+++ b/pkg/ts/query_test.go
@@ -1494,3 +1494,60 @@ func TestQueryWorkerMemoryMonitor(t *testing.T) {
 	t.Logf("total allocations for query: %d\n", memStatsAfter.TotalAlloc-memStatsBefore.TotalAlloc)
 	t.Logf("maximum allocations for query monitor: %d\n", limitedMon.MaximumBytes())
 }
+
+// TestQueryBadRequests confirms that the query method returns gentle errors for
+// obviously bad incoming data.
+func TestQueryBadRequests(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	tm := newTestModel(t)
+	tm.Start()
+	defer tm.Stop()
+
+	account := tm.workerMemMonitor.MakeBoundAccount()
+	defer account.Close(context.TODO())
+
+	// Query with a downsampler that is invalid, expect error.
+	downsampler := (tspb.TimeSeriesQueryAggregator)(999)
+	_, _, err := tm.DB.Query(
+		context.TODO(), tspb.Query{
+			Name:        "metric.test",
+			Downsampler: downsampler.Enum(),
+		}, resolution1ns, 10, 0, 10000, 0, &account, tm.workerMemMonitor,
+	)
+	errorStr := "unknown time series downsampler"
+	if !testutils.IsError(err, errorStr) {
+		if err == nil {
+			t.Fatalf("bad query got no error, wanted to match %q", errorStr)
+		} else {
+			t.Fatalf("bad query got error %q, wanted to match %q", err.Error(), errorStr)
+		}
+	}
+
+	// Query with a aggregator that is invalid, expect error.
+	aggregator := (tspb.TimeSeriesQueryAggregator)(999)
+	_, _, err = tm.DB.Query(
+		context.TODO(), tspb.Query{
+			Name:             "metric.test",
+			SourceAggregator: aggregator.Enum(),
+		}, resolution1ns, 10, 0, 10000, 0, &account, tm.workerMemMonitor,
+	)
+	errorStr = "unknown time series aggregator"
+	if !testutils.IsError(err, errorStr) {
+		if err == nil {
+			t.Fatalf("bad query got no error, wanted to match %q", errorStr)
+		} else {
+			t.Fatalf("bad query got error %q, wanted to match %q", err.Error(), errorStr)
+		}
+	}
+
+	// Query with an interpolator that is invalid, expect no error (default behavior is none).
+	derivative := (tspb.TimeSeriesQueryDerivative)(999)
+	_, _, err = tm.DB.Query(
+		context.TODO(), tspb.Query{
+			Derivative: derivative.Enum(),
+		}, resolution1ns, 10, 0, 10000, 0, &account, tm.workerMemMonitor,
+	)
+	if !testutils.IsError(err, "") {
+		t.Fatalf("bad query got error %q, wanted no error", err.Error())
+	}
+}