HomeWildfire Games

STUN + XMPP ICE implementation.
Needs VerificationrP19703

Description

STUN + XMPP ICE implementation.
Allows lobby players to host games without having to configure their router.

Differential Revision: https://code.wildfiregames.com/D364
Fixes #2305
Patch By: fcxSanya.
StunClient based on code by SuperTuxKart, relicensed with approval of the according authors hilnius, hiker, Auria, deveee, Flakebi, leper, konstin and KroArtem.
Added rfc5245 (ejabberd) support, a GUI option, refactoring and segfault fixes by myself.

Tested By: user1, Sandarac, Sestroretsk1714, Vladislav, Grugnas, javiergodas
Partially Reviewed By: leper, Philip, echotangoecho

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
leper added inline comments.Nov 10 2017, 8:53 AM
/ps/trunk/source/lobby/IXmppClient.h
25

It is declared as a struct.

Reported by clang warning: 'StunEndpoint' defined as a struct here but previously declared as a class [-Wmismatched-tags].

/ps/trunk/source/network/StunClient.cpp
118

This is undefined for u8, which is used below. If you carefully check the upstream code you will notice that they have a different get for u8.

Reported by clang warning: shift count >= width of type [-Wshift-count-overflow].

271

This is the offending instantiation.

Since we can just check the whole thing at once, why not do that instead of looping and invoking undefined behaviour?

Roughly something like the following (completely untested):

if (offset + sizeof(m_TransactionID) >= buffer.size() || memcmp(&buffer[offset], m_TransactionID, sizeof(m_TransactionID)) != 0)
{
    LOGERROR("STUN response doesn't contain the transaction ID");
    return false;
}
offset += sizeof(m_TransactionID);
307

Since you now thought there was only one, this is the other offending one. This is most likely going to need another solution than the above.

Either a different GetFromBuffer specialization for u8, or just if (offset+1 >= buffer.size() || buffer[offset++] != m_IPAddressFamilyIPv4) (also untested).

Thanks for pointing these out, should be an easy fix.

/ps/trunk/source/network/StunClient.cpp
307

In a prior version of this diff there were length checks before each GetFromBuffer call until Philip (IIRC) recommended (2017-05-31) to move them inside the getter, so that it is less likely to use a wrong length number. As I still agree with that argument, I'd prefer specialization.

(Compiling is the non-quick part) SpiderMonkey build fails with a clean working copy after running clean-workspaces.sh, export CC=clang CXX=clang++ and update-workspaces.sh. Maybe I installed a wrong version.

(Compiling is the non-quick part) SpiderMonkey build fails with a clean working copy after running clean-workspaces.sh, export CC=clang CXX=clang++ and update-workspaces.sh. Maybe I installed a wrong version.

You can also just compile the game and not the libs with clang. (At least that's what I did, because well D953 and SpiderMonkey's build system is a mess of scripts and autotools.)

So just do CC=clang CXX=clang++ make -j73 or something like that.

elexis added a comment.EditedNov 11 2017, 1:30 PM

CC=clang CXX=clang++ make -j73
error: input is not a PCH file: 'obj/test_Release/precompiled.h.gch'
fatal error: file 'obj/test_Release/precompiled.h.gch' is not a valid precompiled PCH file

with clang 3.8.0 and gcc 5.4.1 20160904.

So something like that must be running`./update-workspaces.sh --without-pch` prior.

make -j73

So many?

Reproduced, awesome! Adding it to my compile script so I get those clang warnings too and don't forget includes anymore before uploading code.

Hm, still get

make[1]: *** No rule to make target '../../../source/test_root.cpp', needed by 'obj/test_Release/test_root.o'.  Stop.
Makefile:178: recipe for target 'test' failed
make: *** [test] Error 2
Itms added a comment.Nov 11 2017, 6:38 PM

Clang has warnings, warnings are nice. @Itms clang builds for things would be nice.

Nice 🙂

I propose D1026. It has the side effect of renaming workspaces/gcc to workspaces/gmake, I'd need feedback on this.

CC=clang CXX=clang++ make -j73
error: input is not a PCH file: 'obj/test_Release/precompiled.h.gch'
fatal error: file 'obj/test_Release/precompiled.h.gch' is not a valid precompiled PCH file

with clang 3.8.0 and gcc 5.4.1 20160904.

I think you hit D1028. You should clean and update the workspaces before testing my fix. We mustn't disable PCH, especially for automated builds that should be fast!

Thank you for bugging me about the improvement.

So something like that must be running`./update-workspaces.sh --without-pch` prior.

Uh, someone else actually using that.

make -j73

So many?

Be glad I didn't suggest make -j. I did also assume that you used whatever you always use there.

In rP19703#24709, @Itms wrote:

Clang has warnings, warnings are nice. @Itms clang builds for things would be nice.

Nice 🙂

Now I just have to find another issue apart from #2780 to actually get clang+libc++ builds going. :P

I propose D1026. It has the side effect of renaming workspaces/gcc to workspaces/gmake, I'd need feedback on this.

Will have a look.

I think you hit D1028. You should clean and update the workspaces before testing my fix. We mustn't disable PCH, especially for automated builds that should be fast!

Ack, we should still test them sometimes.

elexis added inline comments.Jan 5 2018, 5:43 PM
/ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
229

As reported in D1196, the function is not defined when compiled --without-lobby.

elexis added inline comments.
/ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
1075

@fcxSanya This exposes the real JID to everyone, so it implies to be a non-anonymous room, right? https://xmpp.org/extensions/xep-0045.html#enter-nonanon

In particular from https://xmpp.org/extensions/xep-0045.html#security-anon

If real JIDs are exposed to all occupants in the room, the service MUST warn the user by returning a status code of "100" with the user's initial presence, and the user's client MUST warn the user (a user's client SHOULD also query the room for its configuration prior to allowing the user to enter in order to "pre-discover" whether real JIDs are exposed in the room). A client MUST also warn the user if the room's configuration is modified from semi-anonymous to non-anonymous (which the client will discover when the room sends status code 172).

Currently the room is configured to be anonymous and that technically works. But it doesn't seem to implement what was intended by the specs?

So as mentioned in https://code.wildfiregames.com/rP21720#31523,
README.md should state that rooms should be configured as non-anonymous for this reason.

(Consecutively bot admin rights can be stripped too for security purposes, but that's secondary.)

fcxSanya added inline comments.Oct 29 2018, 8:39 PM
/ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
1075

@elexis the purpose of these specifications is to protect the users privacy when joining general-purpose rooms via generic xmpp clients.
But in our case we use a subset of xmpp features with isolated accounts (we don't xmpp federation right?) and a custom special-purpose client (built-in into the game). The rooms feature allows to share presence information and to actually chat via standard xmpp functionality, but the gamelist isn't directly related to it: our custom client is sharing some information with another client (the lobby bot) about itself and the match and then yet another clients (other players) discovering this information via the bot.
The documentation says that making the room non-anonymous forces the service to automatically share the users JID with everyone else, if we aren't going to use this somehow (for technical purposes), I don't see why we should enable it.

elexis added inline comments.Oct 29 2018, 11:44 PM
/ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
1075

If the room is setup as a non-anonymous room, clients are informed that their real JID is exposed. If it's anonymous they aren't informed.
The real JID is shared by this STUN feature, so the anonymity is out of the window, therefore it would seem more appropriate if the client is warned when entering the room, no?

One benefit when using non-anomyous rooms, that xpartamupp doesn't need muc administrative access anymore to see the real JID of players, so lobby bot bugs wouldn't result in exploits. (But that is independent from the first point.)

Vice versa, I think if we want to support semi-anonymous rooms, the real JID should not be exposed in this line of code, but the anonymous JID.

elexis added inline comments.Oct 29 2018, 11:48 PM
/ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
1075

The real JID is username@lobby.wildfiregames.com/0ad.
The anonymous JID is arena23@conference.lobby.wildfiregames.com/nickname.

JoshuaJB accepted this commit.Jul 17 2019, 9:55 PM
JoshuaJB added a subscriber: JoshuaJB.

D2090 brought to my attention that a few outstanding comments on this were never addressed. I've annotated the remaining comments with where they were fixed and opened D2093 to handle the remaining problems.

I also took another close look at GetFromBuffer and the STUN response parser since they directly deal with unsanitized network data. They seem fine to me (although it's not very nice to rely on the undefined behavior of || evaluations).

/ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
229

This was fixed by D1196.

/ps/trunk/source/lobby/IXmppClient.h
25

For those looking at this in the future, this was fixed in rP20582.

/ps/trunk/source/lobby/XmppClient.cpp
140

Being fixed by D2093.

/ps/trunk/source/lobby/glooxwrapper/glooxwrapper.cpp
291

gloox doesn't seem to document the ownership contract here, but from how we implement this function in XmppClient, the caller should hold ownership. Being fixed in D2093.

800

This is unused. Being removed in D2093.

820

This is actually okay. sessionInitiate takes ownership of this.

/ps/trunk/source/lobby/glooxwrapper/glooxwrapper.h
639

This is being fixed in D2090.

/ps/trunk/source/network/StunClient.cpp
118

This was also fixed in rP20582.

307

This was fixed in the final version. Length checks were moved inside GetFromBuffer.

Read IRC logs from 2013 to understand why this file exists, looked through every line of this diff and some other glooxwrapper functions numerous times, read the gloox code and came up with D2094 in parallel with Joshs D2093 to fix the reported leaks, identify unreported leaks, prefer references over pointers, remove unused classes and add code comments to prove or point at the evidence when and when not to delete.

/ps/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
353

ugly default

/ps/trunk/source/gui/scripting/ScriptFunctions.cpp
396

Avoidable pointer (see D2094). (The new call is in a different file than the delete call below. One can use a reference here. The downside to that is that it also creates the struct in case of error, but I suppose that is negligible as this is only a single 2 member struct per failed connect attempt)

/ps/trunk/source/lobby/IXmppClient.h
61

(Pointer can be avoided)

/ps/trunk/source/lobby/XmppClient.cpp
140

Leak: Correct, missing delete m_sessionManager; in XmppClient::~XmppClient (first version of D2094 has it too).
(Cleanliness: Pointer avoidable by introducing the default constructor and calling a setter here. Not using a pointer means one can't forget the delete call, and things can still be passed by reference. The wrapper mirroring original gloox API (https://camaya.net/api/gloox/classgloox_1_1Jingle_1_1SessionManager.html) and not using pointer making erasure order harder might speak against using a reference.)

1100

(whitespace fixed by rP19741)
Unnecessary pointer (and thus avoid ENSURE D2094)

1114

(whitespace fixed by rP19741, avoidable pointers D2094)

1120

Needless pointer, deref without check (subject to D2094).

/ps/trunk/source/lobby/XmppClient.h
45

(unavoidable pointer unless introducing a setter function that would not mimic the gloox class, as mentioned in the XmppClient ctor)

85

(Needless pointer)

126

Cleanliness: UNUSED should only occur in the implementation.

(The pointers mandatory as long as the XmppClient class inherits glooxwrapper::Jingle::SessionHandler calling this function and the glooxwrapper::Jingle::Session mimicing gloox::Jingle::Session.)

/ps/trunk/source/lobby/glooxwrapper/glooxwrapper.cpp
291

Leak: It's a virtual void only implemented by the gloox client, i.e. the XmppClient, and that doesn't delete it, so it's leaked and can be put on the stack instead (got that too in D2094).

794

Leak / unused code: This class is unused so it can be deleted. It would have leaked the wrapped Content if it had been instantiated, since it misses the destructor.

800

Leak / Unused code: If the unused class wasn't removed (I did that too in D2094), it would probably be cleaner to return references, or tell the caller to delete the plugins.

820

Leak: This leaks (see also IRC discussion on 2019-07-17 when D2093 and D2094 were created).

sessionInitiate does take the new gloox::Jingle::Content, but candidateList is transferred by reference, so nothing deletes this pointer, and it should be a reference intead of a pointer.

838

Leak: Gotcha: gloox::Jingle::PluginList is transferred by reference to the Content, the Content will delete all items of that list (as figured out by Josh, as Content inherits Plugin() and ~Plugin() deletes the plugin items) but not the list itself, so the list should be on the stack as well.

839

No leak: As mentioned above, ~Plugin() inherited by Content deletes the items of the pluginList, i.e. new gloox::Jingle::ICEUDP.

840

No leak: new Content is deleted according to the function comment This object will be owned and deleted by this Session instance. from https://camaya.net/api/gloox/classgloox_1_1Jingle_1_1Session.html#a3393daf4d262b6cf9e3e4df78dbaec6e although I haven't grasped the according gloox code. If it's not deleted, it would be an upstream leak.

843

Unused class

847

(fixed by rP19741)

868

Leak if it wasn't unused code: missing destructor (but the class is never instantiated in pyrogenesis/0ad).

885

Not a leak, since m_HandlerWrapper is deleted below in the destructor, and the Jingle::SessionHandler is the XmppClient which is deleted when leaving the lobby in JSI_Lobby::StopXmppClient.

898

Not a leak, because (from D2094):

This calls m_factory.registerPlugin (see jinglesessionmanager.cpp), hence
~PluginFactory() will delete these new plugin templates.

904

No leak here, proposed comment in D2094:

// The wrapped gloox SessionManager keeps track of this session and deletes it on ~SessionManager()

and

// Hence this may not own the glooxSession, otherwise double-free

/ps/trunk/source/lobby/glooxwrapper/glooxwrapper.h
61

Some always take ownership of their wrapped gloox object

(Those sound like they could have used references instead of pointers if it wasn't mandatory to use m_Wrapped and m_Owned.)

605

unused class (proposed erasure in D2094)

612

Unused class, except for Candidate struct type (removed in D2094).

639

Leak: The variable is unused because the destructor ~Session(); is missing and should use that to delete m_Wrapped (included in D2094).

657

(Unnecessary pointer, can use reference, D2094)

/ps/trunk/source/network/NetClient.cpp
162

(Necessary pointer, since it is only passed for STUN but null and instantiated otherwise in CNetClientSession::Connect.)

/ps/trunk/source/network/NetClient.h
104

(default values in the implementation, not header, or even better not using default values, subject to D2094)

/ps/trunk/source/network/StunClient.cpp
129

(Cleanliness: Needless pass by pointer instead of pass by reference. ENSURE can be avoided, refs D2094)

157

reinterpret_cast

166

Needless pass by pointer instead of pass by reference, D2094

188

reinterpret_cast, static_cast, reinterpret_cast

194

Needless pass by pointer instead of pass by reference, D2094

207

reinterpret_cast

217

reinterpret_cast

226

static_cast

242

reinterpret_cast

271

(fixed by rP20582)

307

Was fixed by rP20582 (also declaring something final rules out future improvements)

353

unnecessary pointer

388

JS_NewPlainObject, subject to D2080

407

(This does not leak because there is a delete call after the only call to this function, but it seems cleaner to not use a pointer to begin with, but a reference.)

410

unnecessary pointer

/ps/trunk/source/network/StunClient.h
40

(unnecessary pointer * 3)

elexis requested verification of this commit.Jul 19 2019, 2:42 PM
elexis removed an auditor: leper.

Commits impending for D2093 and D2094, thanks for the reports and feedback!

This commit now requires verification by auditors.Jul 19 2019, 2:42 PM