Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix the possible overflow case in the current implementation #1473

Closed
wants to merge 2 commits into from

Conversation

xieyuschen
Copy link

@xieyuschen xieyuschen commented Mar 18, 2024

Possible Overflow

Currently, the Add API of Counter and the similar implementation has the overflow problem. I write a test case to demonstrate it by the test case TestCounterAddExcess in the commit 3227fc0. And the output verifies the overflow does happen as expected.

➜  client_golang git:(3227fc0) go version
go version go1.21.3 darwin/arm64
➜  client_golang git:(3227fc0) go test ./prometheus
--- FAIL: TestCounterAddExcess (0.00s)
    counter_test.go:46: oops! overflow
    counter_test.go:63: failed because the internal overflow
    counter_test.go:69: verify the overflow case

Proposal

I think it's nice to fix it as it's a possible case. I haven't checked much about the history discussions about this topic. As a result, I would like to discuss with the maintainers to see whether you would like to accept such changes or not.

We need to check two additional cases for the possible overflows:

  • the input float v is large enough so it will lose precise during casting by uint.
  • the addition overflows

One step further, we can add the valInt value into the float set valBits and then reset the valInt to zero.

Regards.

@xieyuschen xieyuschen changed the title proposal: fix the possible overflow case in the current implementation fix: fix the possible overflow case in the current implementation Mar 18, 2024
@xieyuschen
Copy link
Author

i just found that the casting works different in in linux and macos. will check further tomorrow, so close it.

@xieyuschen xieyuschen closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant