Fixed int64 overflow for timestamp in v1/api parseDuration and parseTime #2501
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This led to unexpected results on wrong query with "(...)&start=148966367200.372&end=1489667272.372" That query is wrong because of `start > end` but actually internal int64 overflow caused start to be something around MinInt64 (huge negative value) and was passing validation. BTW: Not sure if negative timestamp makes sense even.. But model.Earliest is actually MinInt64, can someone explain me why? Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
juliusv
reviewed
Mar 16, 2017
web/api/v1/api_test.go
Outdated
| @@ -243,6 +243,17 @@ func TestEndpoints(t *testing.T) { | |||
| }, | |||
| errType: errorBadData, | |||
| }, | |||
| // Start overflows int64 internally | |||
There was a problem hiding this comment.
End all comments with period please.
|
Thanks, looks good besides nit.
Negative timestamps simply indicate times before Jan 1, 1970. |
juliusv
reviewed
Mar 16, 2017
web/api/v1/api.go
Outdated
| return model.TimeFromUnixNano(ts), nil | ||
| ts := t * float64(time.Second) | ||
| if ts >= float64(math.MaxInt64) || ts <= float64(math.MinInt64) { | ||
| return 0, fmt.Errorf("cannot parse %q to a valid timestamp. It overflows int64", s) |
There was a problem hiding this comment.
Shouldn't this be < and > and not <= and >=?
There was a problem hiding this comment.
I think it does not make any difference, but I agree that it will be never equal (:
There was a problem hiding this comment.
Actually, you are right, it makes difference (: My bad. No error should happen on equal....
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
|
👍 Thanks! |
This was referenced Mar 16, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This led to unexpected results on wrong query with
(...)&start=148966367200.372&end=1489667272.372fields. (additional00at the end of start)That query is wrong because now
start > end, but actually internal int64 overflow caused start to be something aroundMinInt64(huge negative number) and was passing theend.Before(start)validation.In my opinion it should return error on API.
Changes:
ParseTimeandParseDuration, stictly onParseFloatpath.Any feedback welcome!
@brian-brazil @fabxc
BTW: Not sure if negative timestamp makes sense even.. But
model.Earliestis actuallyMinInt64(https://github.com/prometheus/common/blob/master/model/time.go#L36)Can someone explain me why Prometheus time model supports negative timestamps?
Signed-off-by: Bartek Plotka bwplotka@gmail.com