Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use same source address for UDP packets that is used for TCP packets #2623

Merged
merged 3 commits into from Nov 13, 2016

Conversation

@mkrautz
Copy link
Member

commented Nov 5, 2016

This PR changes ServerHandler to bind our UDP socket to the same address that our TCP socket is bound to locally.

Murmur actually requires this for UDP to work, due to the way checkDecrypt in Murmur works.
See:

if (u) {
if (! checkDecrypt(u, encrypt, buffer, len)) {
continue;
}
} else {
// Unknown peer
foreach(ServerUser *usr, qhHostUsers.value(ha)) {
if (checkDecrypt(usr, encrypt, buffer, len)) { // checkDecrypt takes the User's qrwlCrypt lock.
// Every time we relock, reverify users' existance.
// The main thread might delete the user while the lock isn't held.
unsigned int uiSession = usr->uiSession;
rl.unlock();
qrwlVoiceThread.lockForWrite();
if (qhUsers.contains(uiSession)) {
u = usr;
u->sUdpSocket = sock;
memcpy(& u->saiUdpAddress, &from, sizeof(from));
qhHostUsers[from].remove(u);
qhPeerUsers.insert(key, u);
}
qrwlVoiceThread.unlock();
rl.relock();
if (u != NULL && !qhUsers.contains(uiSession))
u = NULL;
break;
}
}
if (! u) {
continue;
}
}

It only tries to look up potential users by the address they're mapped to in qhUsers, which is the TCP address that user is connected with.

This PR also adds a config option. This is to allow users to switch back to the old behavior, if necessary.

This fixes some of the problems raised in #1377. Before this change, Murmur would immediately switch to using a new temporary address as the source address for UDP packets, once a new preferred temporary address was available.

That is, for IPv6 privacy addresses, in a scenario where a given temporary address has a validity of 1 day, but a preferred lifetime of only 4 hours, Mumble would keep connected (via TCP) for the whole day, but voice would only work during the initial 4 hour window. After the 4 hours, Mumble would switch to using whichever new temporary address was made available to it (or even a regular address... we let the kernel decide.)

@Kissaki

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

I can see we bind to the same local address now. But how does that change the behaviour of the UDP socket? Wouldn’t the same problem still occur once the local hostname expires, UDP switch but TCP not?
Or did we effectively bind to different addresses before, which have different behaviour?

@Kissaki
Kissaki approved these changes Nov 5, 2016
Copy link
Member

left a comment

Apart from my concept/logic question, code-wise LGTM.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2016

I can see we bind to the same local address now. But how does that change the behaviour of the UDP socket? Wouldn’t the same problem still occur once the local hostname expires, UDP switch but TCP not?
Or did we effectively bind to different addresses before, which have different behaviour?

The scenario is this:

For TCP, we bind to whichever address the kernel gives picks for the connection.
For UDP, we used to do the same. (Bind to the "Any"/"AnyV6" address.)

In general, this "works". But it breaks in subtle ways when you use IPv6 privacy addresses.

An IPv6 privacy address has a "preferred lifetime" (default 4h in Ubuntu) and a "valid lifetime" (default 24h in Ubuntu).

When you start up your Mumble client and connect to an IPv6 server, it'll use the privacy address (in a typical configuration, at least -- the kernel can be configured to not prefer the temporary privacy addresses).

(5 hours later)...

At this point, the "preferred lifetime" of the privacy address has expired. At this point, the kernel has created a new privacy address that is now "preferred". But the existing address will still be available for a good ~19h. It just won't be used for new connections.

Now comes the "unfortunate" part:

Since TCP is connection-oriented, even though the original privacy address has expired, the connection is still there -- and it will be for another ~19h. Good. And: it still uses the original privacy address.

For UDP, it's a different story. Since we used the "Any" address, at this point any UDP packets we send will be sent with a source address of the new preferred privacy address.

Now there's a mismatch between the TCP source address and the UDP source address, and Murmur doesn't like that.

Voice packets won't be delivered.

What this patch does is change the UDP address we bind to, to be the same as the TCP address.

The result is both the TCP and the UDP sockets now keep working until the address is fully expired, when the validity lifetime has passed.
That's where the other PR, #2622, comes into play. When it detects the connection is gone, it'll try to re-establish it ASAP.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

Ah, because even if we use a UDP socket object, UDP is connection-less, and so new packets are sent with the new address. Makes sense.

@mkrautz mkrautz force-pushed the mkrautz:udpforcetcpaddr branch from 739aff4 to 2b1c6b4 Nov 13, 2016

@mkrautz mkrautz merged commit b2e37e6 into mumble-voip:master Nov 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.