Skip to content
Merged
18 changes: 16 additions & 2 deletions source/common/upstream/thread_aware_lb_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ namespace {
void normalizeHostWeights(const HostVector& hosts, double normalized_locality_weight,
NormalizedHostWeightVector& normalized_host_weights,
double& min_normalized_weight, double& max_normalized_weight) {
uint32_t sum = 0;
// sum should be at most uint32_t max value, so we can validate it by accumulating into unit64_t
// and making sure there was no overflow
uint64_t sum = 0;
for (const auto& host : hosts) {
sum += host->weight();
if (sum > std::numeric_limits<uint32_t>::max()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like a fine way to check for overflow (the other one I could think of would be if (sum + host->weight() >= sum) with sum being uint32_t).

Can you make sure both this and the other exception has coverage and if not add a unit test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @snowp !
I agree that we can check if sum is increasing, but I think the current option clarifies that we check for an overflow.

throw EnvoyException(
fmt::format("Load balancing wieght has accumulated over its maximum value {}",
std::numeric_limits<uint32_t>::max()));
}
}

for (const auto& host : hosts) {
Expand All @@ -31,9 +38,16 @@ void normalizeLocalityWeights(const HostsPerLocality& hosts_per_locality,
double& min_normalized_weight, double& max_normalized_weight) {
ASSERT(locality_weights.size() == hosts_per_locality.get().size());

uint32_t sum = 0;
// sum should be at most uint32_t max value, so we can validate it by accumulating into unit64_t
// and making sure there was no overflow
uint64_t sum = 0;
for (const auto weight : locality_weights) {
sum += weight;
if (sum > std::numeric_limits<uint32_t>::max()) {
throw EnvoyException(
fmt::format("Load balancing wieght has accumulated over its maximum value {}",
std::numeric_limits<uint32_t>::max()));
}
}

// Locality weights (unlike host weights) may be 0. If _all_ locality weights were 0, bail out.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.