Ignore malformed candidates #233
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #233 +/- ##
==========================================
+ Coverage 97.55% 97.59% +0.03%
==========================================
Files 12 12
Lines 1393 1411 +18
==========================================
+ Hits 1359 1377 +18
Misses 18 18
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
philipch07
force-pushed
the
pch07/ignore-malformed-candidate
branch
from
a327f17 to
09ab8cb
Compare
|
Looks good to me, the less strict we're the better. |
No, I haven't gotten around to testing this in-depth yet. I added some fuzz tests though to hopefully catch if anything goes wrong, but you do bring up an important point to make sure we don't panic. If you want to help add more tests for that it would be very welcome!! |
|
@philipch07 panic in SDP isn't the concern here, how webrtc and ICE down the stack handles these malformed candidates, Do you know? |
My current understanding from reading the discussion in the linked issues is that the SDP originally threw errors on malformed/unrecognized candidates which caused subsequent candidates to not be parsed, is that correct? I'm not sure if that answers your question though. |
This is true, now if we make the parser less strict, we should also check that we don't have exceptions in other parts of the stack that can case it to panic. It can be something simple as: reading from an empty slice. |
Oh yeah that's a perfect example; we should definitely check for that. I won't be able to work on this until later tomorrow if you'd like to work on this. No worries if you're busy of course, and no pressure as always. |
|
@philipch07 I'll try to check ICE after i finish something, thank you. |
Awesome! I'm looking forward to reading up on what you come up with tomorrow!! Thanks so much |
philipch07
force-pushed
the
pch07/ignore-malformed-candidate
branch
from
09ab8cb to
bfe121f
Compare
| func isBadCandidateAddress(candidateValue string) bool { | ||
| fields := strings.Fields(candidateValue) | ||
|
|
||
| if len(fields) < 6 { | ||
| return true // clearly malformed candidate line | ||
| } | ||
|
|
||
| addr := fields[4] | ||
|
|
||
| if strings.HasSuffix(addr, ".local") { | ||
| return false // always allow mDNS host candidates | ||
| } | ||
|
|
||
| return net.ParseIP(addr) == nil | ||
| } | ||
|
|
There was a problem hiding this comment.
now that we plan to support hostname candidates, we should just allow them, and maybe ignore them at the ICE layer for now.
philipch07
force-pushed
the
pch07/ignore-malformed-candidate
branch
from
bfe121f to
daecd97
Compare
|
I think this should be done in webrtc :) |
Makes sense, thank you: pion/webrtc#3297 |
2 participants
Description
Ignore malformed candidates instead of returning an error.
Reference issue
Resolves pion/webrtc#1315. This is related to #216, but does not resolve it.