From 5cf5bb427a422082b5a484bae2d8c29dd0bb9241 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Sat, 5 Nov 2016 00:48:32 +0100 Subject: [PATCH 1/2] Check for int64 overflow when converting from float64 --- promql/engine.go | 37 ++++++++++++++++++++++++---------- promql/testdata/operators.test | 5 +++-- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index c25fb51140..82548e0c4f 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -30,6 +30,18 @@ import ( "github.com/prometheus/prometheus/util/stats" ) +const ( + // The largest SampleValue that can be converted to an int64 without overflow. + maxInt64 model.SampleValue = 9223372036854774784 + // The smallest SampleValue that can be converted to an int64 without underflow. + minInt64 model.SampleValue = -9223372036854775808 +) + +// convertibleToInt64 returns true if v does not over-/underflow an int64. +func convertibleToInt64(v model.SampleValue) bool { + return v <= maxInt64 && v >= minInt64 +} + // sampleStream is a stream of Values belonging to an attached COWMetric. type sampleStream struct { Metric metric.Metric @@ -585,9 +597,12 @@ func (ev *evaluator) evalVector(e Expr) vector { } // evalInt attempts to evaluate e into an integer and errors otherwise. -func (ev *evaluator) evalInt(e Expr) int { +func (ev *evaluator) evalInt(e Expr) int64 { sc := ev.evalScalar(e) - return int(sc.Value) + if !convertibleToInt64(sc.Value) { + ev.errorf("scalar value %v overflows int64", sc.Value) + } + return int64(sc.Value) } // evalFloat attempts to evaluate e into a float and errors otherwise. @@ -1022,8 +1037,8 @@ func scalarBinop(op itemType, lhs, rhs model.SampleValue) model.SampleValue { case itemPOW: return model.SampleValue(math.Pow(float64(lhs), float64(rhs))) case itemMOD: - if int(rhs) != 0 { - return model.SampleValue(int(lhs) % int(rhs)) + if int64(rhs) != 0 && convertibleToInt64(lhs) && convertibleToInt64(rhs) { + return model.SampleValue(int64(lhs) % int64(rhs)) } return model.SampleValue(math.NaN()) case itemEQL: @@ -1056,8 +1071,8 @@ func vectorElemBinop(op itemType, lhs, rhs model.SampleValue) (model.SampleValue case itemPOW: return model.SampleValue(math.Pow(float64(lhs), float64(rhs))), true case itemMOD: - if int(rhs) != 0 { - return model.SampleValue(int(lhs) % int(rhs)), true + if int64(rhs) != 0 && convertibleToInt64(lhs) && convertibleToInt64(rhs) { + return model.SampleValue(int64(lhs) % int64(rhs)), true } return model.SampleValue(math.NaN()), true case itemEQL: @@ -1099,7 +1114,7 @@ type groupedAggregation struct { func (ev *evaluator) aggregation(op itemType, grouping model.LabelNames, without bool, keepCommon bool, param Expr, vec vector) vector { result := map[uint64]*groupedAggregation{} - var k int + var k int64 if op == itemTopK || op == itemBottomK { k = ev.evalInt(param) if k < 1 { @@ -1202,15 +1217,15 @@ func (ev *evaluator) aggregation(op itemType, grouping model.LabelNames, without groupedResult.valuesSquaredSum += s.Value * s.Value groupedResult.groupCount++ case itemTopK: - if len(groupedResult.heap) < k || groupedResult.heap[0].Value < s.Value || math.IsNaN(float64(groupedResult.heap[0].Value)) { - if len(groupedResult.heap) == k { + if int64(len(groupedResult.heap)) < k || groupedResult.heap[0].Value < s.Value || math.IsNaN(float64(groupedResult.heap[0].Value)) { + if int64(len(groupedResult.heap)) == k { heap.Pop(&groupedResult.heap) } heap.Push(&groupedResult.heap, &sample{Value: s.Value, Metric: s.Metric}) } case itemBottomK: - if len(groupedResult.reverseHeap) < k || groupedResult.reverseHeap[0].Value > s.Value || math.IsNaN(float64(groupedResult.reverseHeap[0].Value)) { - if len(groupedResult.reverseHeap) == k { + if int64(len(groupedResult.reverseHeap)) < k || groupedResult.reverseHeap[0].Value > s.Value || math.IsNaN(float64(groupedResult.reverseHeap[0].Value)) { + if int64(len(groupedResult.reverseHeap)) == k { heap.Pop(&groupedResult.reverseHeap) } heap.Push(&groupedResult.reverseHeap, &sample{Value: s.Value, Metric: s.Metric}) diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index d9e96fa932..c14386aa97 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -54,9 +54,10 @@ eval instant at 50m SUM(http_requests) BY (job) % 2 ^ 3 ^ 2 {job="api-server"} 488 {job="app-server"} 40 +# int64 overflow on RHS of %. eval instant at 50m SUM(http_requests) BY (job) % 2 ^ 3 ^ 2 ^ 2 - {job="api-server"} 1000 - {job="app-server"} 2600 + {job="api-server"} NaN + {job="app-server"} NaN eval instant at 50m COUNT(http_requests) BY (job) ^ COUNT(http_requests) BY (job) {job="api-server"} 256 From 4e3abc6cbf7f1d25499f9be0a9838683b0754468 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 8 Nov 2016 21:03:31 +0100 Subject: [PATCH 2/2] Simply use `math.Mod(float64, float64)` after all This circumvents all the problems with int overflow, plus it is what was originally intended. --- promql/engine.go | 10 ++-------- promql/testdata/operators.test | 19 ++++--------------- 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 82548e0c4f..253dc42454 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -1037,10 +1037,7 @@ func scalarBinop(op itemType, lhs, rhs model.SampleValue) model.SampleValue { case itemPOW: return model.SampleValue(math.Pow(float64(lhs), float64(rhs))) case itemMOD: - if int64(rhs) != 0 && convertibleToInt64(lhs) && convertibleToInt64(rhs) { - return model.SampleValue(int64(lhs) % int64(rhs)) - } - return model.SampleValue(math.NaN()) + return model.SampleValue(math.Mod(float64(lhs), float64(rhs))) case itemEQL: return btos(lhs == rhs) case itemNEQ: @@ -1071,10 +1068,7 @@ func vectorElemBinop(op itemType, lhs, rhs model.SampleValue) (model.SampleValue case itemPOW: return model.SampleValue(math.Pow(float64(lhs), float64(rhs))), true case itemMOD: - if int64(rhs) != 0 && convertibleToInt64(lhs) && convertibleToInt64(rhs) { - return model.SampleValue(int64(lhs) % int64(rhs)), true - } - return model.SampleValue(math.NaN()), true + return model.SampleValue(math.Mod(float64(lhs), float64(rhs))), true case itemEQL: return lhs, lhs == rhs case itemNEQ: diff --git a/promql/testdata/operators.test b/promql/testdata/operators.test index c14386aa97..2807991f3f 100644 --- a/promql/testdata/operators.test +++ b/promql/testdata/operators.test @@ -35,8 +35,8 @@ eval instant at 50m SUM(http_requests) BY (job) % 3 {job="app-server"} 2 eval instant at 50m SUM(http_requests) BY (job) % 0.3 - {job="api-server"} NaN - {job="app-server"} NaN + {job="api-server"} 0.1 + {job="app-server"} 0.2 eval instant at 50m SUM(http_requests) BY (job) ^ 2 {job="api-server"} 1000000 @@ -54,10 +54,9 @@ eval instant at 50m SUM(http_requests) BY (job) % 2 ^ 3 ^ 2 {job="api-server"} 488 {job="app-server"} 40 -# int64 overflow on RHS of %. eval instant at 50m SUM(http_requests) BY (job) % 2 ^ 3 ^ 2 ^ 2 - {job="api-server"} NaN - {job="app-server"} NaN + {job="api-server"} 1000 + {job="app-server"} 2600 eval instant at 50m COUNT(http_requests) BY (job) ^ COUNT(http_requests) BY (job) {job="api-server"} 256 @@ -352,13 +351,3 @@ eval instant at 5m metricA + ignoring() metricB eval instant at 5m metricA + metricB {baz="meh"} 7 - -clear - -load 5m - finite{foo="bar"} 42 - almost_zero{foo="bar"} 0.123 - -# MOD by "almost zero" with vector. -eval instant at 5m finite % almost_zero - {foo="bar"} NaN \ No newline at end of file