MUC bytestreams may not check if data is successfully sent?

Submitted by Will Thompson @wjt

Assigned to Telepathy bugs list

Link to original bug (#47999)

Description

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

Version: git master