Skip to content

balancer/ringhash: fix uint32 weight sum overflow in normalizeWeights - #9357

Open
nvxbug wants to merge 2 commits into
grpc:masterfrom
nvxbug:ringhash-weight-sum-overflow
Open

balancer/ringhash: fix uint32 weight sum overflow in normalizeWeights#9357
nvxbug wants to merge 2 commits into
grpc:masterfrom
nvxbug:ringhash-weight-sum-overflow

Conversation

@nvxbug

@nvxbug nvxbug commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

newRing with two endpoints whose uint32 weights sum to 2^32 (e.g. an EDS locality weight of 2 times an endpoint weight of 2^30, which pass the per-locality and per-priority weight-sum checks):

ring.go: normalized endpoint weights is [{a +Inf ...} {b +Inf ...}]
ring.go: creating new ring of size 1024
(hangs)

normalizeWeights sums endpoint weights into a uint32, so a total above math.MaxUint32 wraps and can land on zero. The weight / weightSum divide then yields +Inf, newRing sets targetHashes to +Inf, and its for currentHashes < targetHashes loop never ends, growing the ring until the process is out of memory. Summing in uint64 cannot wrap and stays non-zero, since every endpoint weight is at least 1.

RELEASE NOTES:

  • balancer/ringhash: Fix a bug where sum of endpoint exceeding math.MaxUint32 will get wrapped by storing the weight it uint64

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.55%. Comparing base (4793ad0) to head (d55cb28).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9357      +/-   ##
==========================================
+ Coverage   87.42%   87.55%   +0.12%     
==========================================
  Files         424      429       +5     
  Lines       30283    30622     +339     
==========================================
+ Hits        26476    26812     +336     
- Misses       3806     3809       +3     
  Partials        1        1              
Files with missing lines Coverage Δ
balancer/ringhash/ring.go 100.00% <100.00%> (ø)

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread balancer/ringhash/ring_test.go Outdated
Comment thread balancer/ringhash/ring.go Outdated
Comment thread balancer/ringhash/ring_test.go
Check exact 0.5 distribution in overflow test, add a test for a sum
that wraps to a non-zero value, and expand the accumulator comment to
cover both wrap outcomes.

@eshitachandwani eshitachandwani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with 2 small comments about test comments. Adding @easwars as a second reviewer.

}
}

// TestRingNewWeightSumOverflow checks that endpoint weights whose sum exceeds

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit : can we reword this a little to make it more readable , something along the lines of ... Tests the scenario where the sum of endpoint weights exceed math.MaxUint32 and would have been wrapped to zero using a unit32. Verifies that ring-build loop does not go on forever and produces a correct ring.
Or something more clear.


// TestRingNewWeightSumOverflowToNonZero checks that endpoint weights whose sum
// exceeds math.MaxUint32 and wraps to a non-zero value still produce a ring
// within the configured size bounds. A wrapped non-zero sum used to make

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need not write what the code used to do , rather we can write something like using uint32 can wrap it to a smaller value....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants