[Bug 47999] New: in-band and MUC bytestreams doesn't check if data is successfully sent

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=47999

             Bug #: 47999
           Summary: in-band and MUC bytestreams doesn't check if data is
                    successfully sent
    Classification: Unclassified
           Product: Telepathy
           Version: git master
          Platform: Other
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: medium
         Component: gabble
        AssignedTo: telepathy-bugs at lists.freedesktop.org
        ReportedBy: will.thompson at collabora.co.uk
         QAContact: telepathy-bugs at lists.freedesktop.org


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

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.