Bug 47999 - MUC bytestreams may not check if data is successfully sent?
Summary: MUC bytestreams may not check if data is successfully sent?
Status: NEW
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard: libreoffice-tubes
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-28 05:41 UTC by Will Thompson
Modified: 2012-11-05 14:01 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Will Thompson 2012-03-28 05:41:41 UTC
During the LibreOffice hackfest, Michael and Eike asked me how reliable tubes are. Specifically, might we ever drop packets? I said I thought that if a message failed to send for whatever reason (maybe rate-limiting by the server) the tube would die, which is imperfect but at least means that as long as the tube is open it's reliable.

I was wrong, at least for in-band bytestreams.

In bytestream-ibb.c's send_data() function, an IQ is constructed with the data to send, and sent (irrelevant bookkeeping elided):


      iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_SUB_TYPE_SET,
          NULL, priv->peer_jid,
          '(', "data",
            '$', encoded,
            ':', NS_IBB,
            '@', "sid", priv->stream_id,
            '@', "seq", seq,
          ')', NULL);

      ret = _gabble_connection_send_with_reply (priv->conn, iq, iq_acked_cb,
          G_OBJECT (self), NULL, &error);

      if (!ret)
        {
          gabble_bytestream_iface_close (GABBLE_BYTESTREAM_IFACE (self), NULL);

_gabble_connection_send_with_reply() succeeds unless you're literally not connected any more, so the error-checking here is perhaps a little overly keen. But iq_acked_cb doesn't look at the reply to see if it's got type='result' or type='error'. If the IQ is sent successfully but we get an error reply back, we blindly carry on sending subsequent chunks of data. Badness.

(If sending the IQ fails (that is, if we disconnect before we get a reply), the callback is never called. It looks like that's okay in this case.)

I think bytestream-socks5 is okay. bytestream-muc.c uses <message/> stanzas, doesn't put an id='' on them, and doesn't check for errors or for the echo as far as I can tell, so may have similar issues if the MUC rejects some of our messages (rate-limiting doesn't seem unlikely…).
Comment 1 Will Thompson 2012-03-28 07:12:16 UTC
This branch <http://cgit.collabora.com/git/user/wjt/telepathy-gabble/log/?h=47999-handle-ibb-errors> makes the bytestream close up if a stanza gets dropped.

There's no test for this in the branch, because I know Jonny has been gutting the tube tests of references to the old API and I don't want to conflict. I tested this by making the test always send back errors for IBB IQs:

@@ -664,8 +664,7 @@ class BytestreamIBB(Bytestream):
             assert ibb_data['sid'] == self.stream_id
 
             # ack the IQ
-            result = make_result_iq(self.stream, ibb_event.stanza)
-            result.send()
+            send_error_reply(self.stream, ibb_event.stanza)
 
             if len(binary) >= size or size == 0:
                 received = True

Before this branch, tubes/accept-private-stream-tube.py (which tells Gabble to send some data down the bytestream, waits for it to arrive, sends back a reply over XMPP and sucks the reply out of Gabble) passes even with this change. With this branch, this change breaks this and other tests, as you would expect.

This branch doesn't address MUCs.
Comment 2 Jonny Lamb 2012-03-30 12:31:14 UTC
I could get behind this. r+
Comment 3 Will Thompson 2012-04-02 09:52:48 UTC
Merged. http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=4d9133a523d95207699b3ede02b26e7e2c272fdd

Retitling for MUCs maybe having the same issue (which I still haven't really checked.)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.