Fix infinite REGISTER for multi-homed remotes. #13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original code tried to map belle_sip_deb(...) (i.e., a variadic
macro) to bctbx_debug(...) (i.e., another variadic macro as the
replacement).
This didn't work for multiple reasons:
- no code used belle_sip_deb(), but actually belle_sip_debug().
- the target function has to be called with either a named variadic
argument (e.g., "nameargs..." -> "nameargs") or, if it's
unnamed/anonymous via the portable __VA_ARGS__ name/macro.
Rename it to belle_sip_debug and make it a non-variadic substitution to
bctbx_debug, since the usage is 1:1 currently.
If it stops being a 1:1 replacement in the future, it's easy to just
make it variadic again such as via:
belle_sip_debug(args...) some_func(NULL, args)
(Still) disabled by default, of course.
Instead, let's just use the proper format specifier. Since belle-sip already requires C99, it should always be available.
No functional change, of course.
…_contact_address_accurate(). It's static, so nothing other than the compilation unit is affected.
Nothing outside of the compilation unit uses it, so it should have been static since its inception. If it does turn out that it needs to be part of the API, we can still make it non-static and export it in a header file.
…none.
This fixes the infamous "register loop" issue that is, e.g., triggered
by networks with multi-homed hosts which also act as SIP servers.
Consider this example:
- client [10.0.0.2] contacts
server [10.0.0.1 (N.B.: private), 203.0.113.14 (N.B.: public)]
by using [203.0.113.14] explicitly.
- belle-sip channel [NULL, 203.0.113.14] is being created (since the
client-side public IP address can only be discovered on replies).
- server responds via [10.0.0.1] instead.
- belle-sip channel [10.0.0.2, 10.0.0.1] is being created (client-side
public IP address comes from SIP packet information).
- client notices that the pair (NULL, [10.0.0.2]) doesn't match,
assumes that REGISTER failed.
- new REGISTER round starts.
Applying the newly learned public IP address to other channels that have
none set should, in theory, only ever change one channel - the
corresponding sending socket.
Practically, though, depending on factors such as if belle-sip can be
used in a multi-threaded fashion or if multiple connections can be
opened (almost) at the same time and network timing effects, it's not
unthinkable of situations like these:
- client sends REGISTER to server1 and waits => channel1
- client sends REGISTER to server2 and waits => channel2
- server2 answers via different IP address => channel3 => public IP
address passed to channel1 (wrongly) and channel2 (correctly)
- server1 answers via different IP address => channel4
- public IP address pair of channel1 and channel4 won't match =>
infinite REGISTER sequence starts
It's unclear to me if this race can happen, though. Fixing it would be
very difficult, too, since we'd have to synchronize requests and
replies, which currently does not seem to be a concept in belle-sip, or
to match sending and receiving channels, which is not trivially possible
and might even be impossible with more work involved.
See BelledonneCommunications/linphone-sdk#17
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please refer to the commit message for more information of why this is necessary and how it's fixed.
I'd like to highlight the (rather big) caveat here:
If clients can send multiple
REGISTERrequests at once and if one of those requests is answered via a different remote IP address, it's possible that the wrong public IP address is copied to channels that have none set but also shouldn't have it set.That's a bit concerning, but I don't see an easy way to fix that issue.
Additionally, it should be at most an issue for multi-homed clients, so should only come up very rarely. However, keep in mind that a public and private IP address already constitute multi-homedness (for our purposes), so it might as well be more likely yet again.
Fixes: BelledonneCommunications/linphone-sdk#17
Includes #12, so don't review/merge before the other. I'll rebase this PR as soon as #12 is merged.