les/utils: fix panic in weighted select#21821
les/utils: fix panic in weighted select#21821holiman wants to merge 2 commits intoethereum:masterfrom
Conversation
| return nil | ||
| } | ||
| val := uint64(rand.Int63n(int64(w.root.sumWeight))) | ||
| val := rand.Uint64() % w.root.sumWeight |
There was a problem hiding this comment.
Please check the L92, it has the same issue.
The suggested change:
if weight >= lastWeight || rand.Uint64() % lastWeight < weight {
return choice
}
| t.Fatalf("test is dysfunctional, sumweight not negative: %d", int64(s.root.sumWeight)) | ||
| } | ||
| s.Choose() | ||
| } |
There was a problem hiding this comment.
// TestOOB tests values which doesn't fit in int64
func TestOOB(t *testing.T) {
s := NewWeightedRandomSelect(func(i interface{}) uint64 {
// Dummy weight function to return a very large weight
return uint64(0xffffffffffffffff)
})
s.Update(testWrsItem{idx: 0, widx: nil})
// int64 conversion should make the sumweight negative
if int64(s.root.sumWeight) >= 0 {
t.Fatalf("test is dysfunctional, sumweight not negative: %d", int64(s.root.sumWeight))
}
s.Choose()
}This test can capture the very large single weight. For some cornercases.
zsfelfoldi
left a comment
zsfelfoldi
left a comment
There was a problem hiding this comment.
Honestly I don't think this is the right way to fix this. The overflow should not happen in the first place and though this change prevents the selector panic. sumWeight can still overflow and wrap around. This fix basically just hides the issue, causing the client to fail in weird ways without signaling the error. In this particular case it was probably an Inf/NaN float converted to uint64.
I will open another PR that also prevents sumWeight wrap-around and gives a proper error (and does some stupid but safe fallback) if the sum of weights would be too much. Of course I'll also fix the actual bug that has happened here.
3 participants
Fixes #21820