p2p: fix infinite loop in dnsaddr resolution#5926
p2p: fix infinite loop in dnsaddr resolution#5926algorandskiy merged 1 commit intoalgorand:feature/p2pfrom
Conversation
| mockController{}, | ||
| func(dnsaddr multiaddr.Multiaddr, entries []multiaddr.Multiaddr) error { return nil }, | ||
| ) | ||
| return true |
There was a problem hiding this comment.
Ideally this would check the error returned by Iterate
There was a problem hiding this comment.
why? I want to check no infinite loops and not resolution errors
There was a problem hiding this comment.
You made the decision to return an error when max hops are reached. In my view, it only makes sense to test that return value here, rather than ignoring it
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/p2p #5926 +/- ##
===============================================
+ Coverage 55.98% 55.99% +0.01%
===============================================
Files 482 482
Lines 68057 68062 +5
===============================================
+ Hits 38103 38114 +11
+ Misses 27347 27343 -4
+ Partials 2607 2605 -2 ☔ View full report in Codecov by Sentry. |
jasonpaulos
left a comment
jasonpaulos
left a comment
There was a problem hiding this comment.
I left a test suggestion, but it's not serious enough to be a blocker
| mockController{}, | ||
| func(dnsaddr multiaddr.Multiaddr, entries []multiaddr.Multiaddr) error { return nil }, | ||
| ) | ||
| return true |
There was a problem hiding this comment.
You made the decision to return an error when max hops are reached. In my view, it only makes sense to test that return value here, rather than ignoring it
| if resolver == nil { | ||
| return errors.New("passed controller has no resolvers Iterate") | ||
| } | ||
| const maxHops = 100 |
There was a problem hiding this comment.
Why 100 - i.e. this feels more like 10 is sufficient..?
There was a problem hiding this comment.
I do not know what is actual max limit
3 participants
Summary
dnsaddr.Iterate()is not tolerate to cycles. Added max attempts to prevent infinite looping.Test Plan
Added a unit test.