fix index out of bound bug when comparing ZLabelSets (thanos-io#3520)

* fix index out of bound bug when comparing ZLabelSets Signed-off-by: Ben Ye <[email protected]> * fix param parsing error message Signed-off-by: Ben Ye <[email protected]> * address comment feedbacks Signed-off-by: Ben Ye <[email protected]>
metalmatze · Dec 9, 2020 · 12e5c26 · 12e5c26
1 parent 5cbf23e
commit 12e5c26
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 11 deletions.
diff --git a/pkg/queryfrontend/labels_codec.go b/pkg/queryfrontend/labels_codec.go
@@ -84,7 +84,7 @@ func (c labelsCodec) MergeResponse(responses ...queryrange.Response) (queryrange
 			Data:   lbls,
 		}, nil
 	case *ThanosSeriesResponse:
-		seriesData := make([]labelpb.ZLabelSet, 0)
+		seriesData := make(labelpb.ZLabelSets, 0)
 
 		uniqueSeries := make(map[string]struct{})
 		for _, res := range responses {
@@ -283,7 +283,7 @@ func (c labelsCodec) parseLabelsRequest(r *http.Request, op string) (queryrange.
 		return nil, err
 	}
 
-	result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam])
+	result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam)
 	if err != nil {
 		return nil, err
 	}
@@ -317,7 +317,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er
 		return nil, err
 	}
 
-	result.Matchers, err = parseMatchersParam(r.Form[queryv1.MatcherParam])
+	result.Matchers, err = parseMatchersParam(r.Form, queryv1.MatcherParam)
 	if err != nil {
 		return nil, err
 	}
@@ -336,7 +336,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er
 		result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam]
 	}
 
-	result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam])
+	result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/queryfrontend/queryrange_codec.go b/pkg/queryfrontend/queryrange_codec.go
@@ -111,7 +111,7 @@ func (c queryRangeCodec) DecodeRequest(_ context.Context, r *http.Request) (quer
 		result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam]
 	}
 
-	result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam])
+	result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam)
 	if err != nil {
 		return nil, err
 	}
@@ -221,12 +221,12 @@ func parsePartialResponseParam(s string, defaultEnablePartialResponse bool) (boo
 	return defaultEnablePartialResponse, nil
 }
 
-func parseMatchersParam(ss []string) ([][]*labels.Matcher, error) {
-	matchers := make([][]*labels.Matcher, 0, len(ss))
-	for _, s := range ss {
+func parseMatchersParam(ss url.Values, matcherParam string) ([][]*labels.Matcher, error) {
+	matchers := make([][]*labels.Matcher, 0, len(ss[matcherParam]))
+	for _, s := range ss[matcherParam] {
 		ms, err := parser.ParseMetricSelector(s)
 		if err != nil {
-			return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.StoreMatcherParam)
+			return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, matcherParam)
 		}
 		matchers = append(matchers, ms)
 	}

diff --git a/pkg/store/labelpb/label.go b/pkg/store/labelpb/label.go
@@ -307,7 +307,8 @@ func (z ZLabelSets) Less(i, j int) bool {
 	l := 0
 	r := 0
 	var result int
-	for l < z[i].Size() && r < z[j].Size() {
+	lenI, lenJ := len(z[i].Labels), len(z[j].Labels)
+	for l < lenI && r < lenJ {
 		result = z[i].Labels[l].Compare(z[j].Labels[r])
 		if result == 0 {
 			l++
@@ -317,5 +318,5 @@ func (z ZLabelSets) Less(i, j int) bool {
 		return result < 0
 	}
 
-	return l == z[i].Size()
+	return l == lenI
 }
diff --git a/pkg/store/labelpb/label_test.go b/pkg/store/labelpb/label_test.go
@@ -129,6 +129,34 @@ func TestSortZLabelSets(t *testing.T) {
 				}),
 			),
 		},
+		{
+			Labels: ZLabelsFromPromLabels(
+				labels.FromMap(map[string]string{
+					"__name__":  "grpc_client_handled_total",
+					"cluster":   "test",
+					"grpc_code": "OK",
+					"aa":        "1",
+					"bb":        "2",
+					"cc":        "3",
+					"dd":        "4",
+					"ee":        "5",
+				}),
+			),
+		},
+		{
+			Labels: ZLabelsFromPromLabels(
+				labels.FromMap(map[string]string{
+					"__name__":  "grpc_client_handled_total",
+					"cluster":   "test",
+					"grpc_code": "OK",
+					"aa":        "1",
+					"bb":        "2",
+					"cc":        "3",
+					"dd":        "4",
+					"ee":        "5",
+				}),
+			),
+		},
 		{
 			Labels: ZLabelsFromPromLabels(
 				labels.FromMap(map[string]string{
@@ -188,6 +216,35 @@ func TestSortZLabelSets(t *testing.T) {
 				}),
 			),
 		},
+		{
+			Labels: ZLabelsFromPromLabels(
+				labels.FromMap(map[string]string{
+					"__name__":  "grpc_client_handled_total",
+					"cluster":   "test",
+					"grpc_code": "OK",
+					"aa":        "1",
+					"bb":        "2",
+					"cc":        "3",
+					"dd":        "4",
+					"ee":        "5",
+				}),
+			),
+		},
+		// This label set is the same as the previous one, which should correctly return 0 in Less() function.
+		{
+			Labels: ZLabelsFromPromLabels(
+				labels.FromMap(map[string]string{
+					"cluster":   "test",
+					"__name__":  "grpc_client_handled_total",
+					"grpc_code": "OK",
+					"aa":        "1",
+					"bb":        "2",
+					"cc":        "3",
+					"dd":        "4",
+					"ee":        "5",
+				}),
+			),
+		},
 	}
 
 	sort.Sort(list)