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);

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).

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.

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

@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.)

@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.

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.

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

ugly default

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)

(Pointer can be avoided)

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.)

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

(whitespace fixed by rP19741, avoidable pointers D2094)

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

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

(Needless pointer)

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.)

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).

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.

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.

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.

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.

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

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.

Unused class

(fixed by rP19741)

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

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.

Not a leak, because (from D2094):

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

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

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.)

unused class (proposed erasure in D2094)

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

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

(Unnecessary pointer, can use reference, D2094)

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

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

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

reinterpret_cast

Needless pass by pointer instead of pass by reference, D2094

reinterpret_cast, static_cast, reinterpret_cast

Needless pass by pointer instead of pass by reference, D2094

reinterpret_cast

reinterpret_cast

static_cast

reinterpret_cast

(fixed by rP20582)

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

unnecessary pointer

JS_NewPlainObject, subject to D2080

(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.)

unnecessary pointer

(unnecessary pointer * 3)