robbiehanson / XMPPFramework

An XMPP Framework in Objective-C for Mac and iOS
Other
5.91k stars 2.09k forks source link

Connection goes up and down due to improper stanza count #1101

Open GlacierSecurityInc opened 6 years ago

GlacierSecurityInc commented 6 years ago

As we bring up the client, we have noticed the connection bouncing up and down a few times before the connection stays up. After further investigation, this seems to be an issue with XEP-0198 and the way stanzas are being counted.

We are using the latest version of ejabberd (18.0x), I'm not sure if this happens with Prosody. When XMPPFramework reports that it received more stanzas than ejabberd has sent, ejabberd closes the stream and the stream needs to be restarted.

XMPPFramework is properly waiting until after the enable message response from the server before it starts counting stanzas. However, it puts incoming XMPPElements on an async queue. As a result, sometimes it receives an IQ, then receives the enable response, processes the enable response, and then processes the IQ that arrived earlier but was on an async queue.

In other words, it processes the enabled message first, resets its counter and starts counting incoming stanzas. Then it processes the IQ that it actually received before the enabled message. This causes an incorrect stanza count to be sent back to the server.

We have a fix for this, but it seems a bit hacky. I'll be glad to create a pull request, but I'd rather see if someone familiar with the code has a better way to handle this.

To briefly describe, our fix involves adding and setting a "streamMgmtEnabled" flag in XMPPStream that gets set to NO on disconnect and YES when the enabled message is received. We then have a "preEnabled" flag in XMPPElement that gets set to true on an incoming IQ, Message, or Presence if streamMgmtEnabled is false. And finally, we do not add stanzas to the "unackedByClient" list if the preEnabled flag for an XMPPElement is set to true.

GlacierSecurityInc commented 6 years ago

FYI, there are two reasons I said our fix seems a little hacky and I wondered if someone else could come up with something better.

1) Since incoming stanzas are handled by the multicastDelegates, it seems that XMPPStream shouldn't need to know or care about the enabled message, and shouldn't need a flag. However, we had to put the flag there because the stanzas are put on the async queue before XMPPStreamManagement ever sees them.

2) We weren't able to directly add the "preEnabled" property to XMPPElement and instead used an associated object as follows in XMPPElement:

- (void)setPreEnabled:(BOOL)preEnabled
{
    NSNumber *number = [NSNumber numberWithBool: preEnabled];
    objc_setAssociatedObject(self, PreEnabledTagKey, number, OBJC_ASSOCIATION_RETAIN);
}

- (BOOL)preEnabled
{
    NSNumber *number = objc_getAssociatedObject(self, PreEnabledTagKey);
    if (number)
        return [number boolValue];

    return false;
} 
chrisballinger commented 6 years ago

Thanks for the detailed investigation! I agree that the proposed fix is a bit hacky, but the problem is probably hard to solve in a "clean" way regardless of the approach.

Yoni-Reiss commented 2 years ago

Hi, I've also noticed this same bug recently, and I'm very interested in the solution you've described. Can I find the solution anywhere? does it exist in any branch of this repository? I would appreciate any help.