Fix an overflow on retries on container name conflicts #4752
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
|
LGTM |
to reflect a bit better what this does. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Because Go integers silently overflow, and 10 is multiple of 2, repeatedly multiplying by 2 eventually turns suffixDigitsModulo into all-bits-zero, triggering a division by zero. So, cap the suffix to a 9-digit one. That is very likely to be sufficient. (But don't actually limit the number of retries. We could do that as well, for extra robustness. OTOH that would be another bit of code that we don't test...) [NO NEW TESTS NEEDED] We could plausibly unit-test the loop just to see that it doesn't crash on 100 conflicts in a row, but that's an awfully specific test that would require moving the loop to a separate function with a closure parameter, a complex test that makes the code less readable for a very specific crash. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
I did that, with an attempt at a justification in the commit. I can very well see an argument that this should have a unit test anyway, say so if you want me to write one. |
|
LGTM |
|
Thanks! Tests passed now. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What type of PR is this?
What this PR does / why we need it:
Original report #4679 , probably requires a deterministically-seeded RNG.
Because Go integers silently overflow, and 10 is multiple of 2, repeatedly multiplying by 2 eventually turns suffixDigitsModulo
into all-bits-zero, triggering a division by zero.
So, cap the suffix to a 9-digit one. That is very likely to be sufficient.
(But don't actually limit the number of retries. We could do that as well, for extra robustness. OTOH that would be another bit of code that we don't test...)
How to verify it
With a deterministic RNG, and 32-bit integers, about 30 repeated triggers of this code should trigger that overflow. I didn’t try.
Which issue(s) this PR fixes:
Fixes #4679
Special notes for your reviewer:
Does this PR introduce a user-facing change?