Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#15684 closed defect (fixed)

BOSH does not process all responses in buffer

Reported by: flopma Owned by: darkrain42
Milestone: 2.10.8 Component: libpurple
Version: 2.10.7 Keywords: bosh
Cc:

Description

Hi,

For pidgin connected via BOSH/XMPP.

If 2 or more server replies are read inside function jabber_bosh_http_connection_process(PurpleHTTPConnection *conn) only the first reply will be processed by function http_received_cb().

For example, when Pidgin reads

(14:21:48) jabber: BOSH server sent: HTTP/1.1 200 OK
Date: Fri, 05 Jul 2013 12:20:48 GMT
Server: Jetty(7.x.y-SNAPSHOT)
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: PROPFIND, PROPPATCH, COPY, MOVE, DELETE, MKCOL, LOCK, UNLOCK, PUT, GETLIB, VERSION-CONTROL, CHECKIN, CHECKOUT, UNCHECKOUT, REPORT, UPDATE, CANCELUPLOAD, HEAD, OPTIONS, GET, POST
Access-Control-Allow-Headers: Overwrite, Destination, Content-Type, Depth, User-Agent, X-File-Size, X-Requested-With, If-Modified-Since, X-File-Name, Cache-Control
Access-Control-Max-Age: 86400
Content-Type: text/xml;charset=UTF-8
Content-Length: 57
Proxy-Connection: Keep-Alive
Connection: Keep-Alive

<body xmlns="http://jabber.org/protocol/httpbind"></body>HTTP/1.1 200 OK
Date: Fri, 05 Jul 2013 12:21:48 GMT
Server: Jetty(7.x.y-SNAPSHOT)
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: PROPFIND, PROPPATCH, COPY, MOVE, DELETE, MKCOL, LOCK, UNLOCK, PUT, GETLIB, VERSION-CONTROL, CHECKIN, CHECKOUT, UNCHECKOUT, REPORT, UPDATE, CANCELUPLOAD, HEAD, OPTIONS, GET, POST
Access-Control-Allow-Headers: Overwrite, Destination, Content-Type, Depth, User-Agent, X-File-Size, X-Requested-With, If-Modified-Since, X-File-Name, Cache-Control
Access-Control-Max-Age: 86400
Content-Type: text/xml;charset=UTF-8
Content-Length: 152
Proxy-Connection: Keep-Alive
Connection: Keep-Alive

<body xmlns='http://jabber.org/protocol/httpbind'><iq xmlns="jabber:client" type="result" id="purplef3ce4cf9" to="gorisis@xmpp.citnet/98a19540"/></body>
(14:21:48) jabber: RecvBOSH (ssl)(57): <body xmlns="http://jabber.org/protocol/httpbind"></body>HTTP/1.1 200 OK
Date: Fri, 05 Jul 2013 12:21:48 GMT
Server: Jetty(7.x.y-SNAPSHOT)
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: PROPFIND, PROPPATCH, COPY, MOVE, DELETE, MKCOL, LOCK, UNLOCK, PUT, GETLIB, VERSION-CONTROL, CHECKIN, CHECKOUT, UNCHECKOUT, REPORT, UPDATE, CANCELUPLOAD, HEAD, OPTIONS, GET, POST
Access-Control-Allow-Headers: Overwrite, Destination, Content-Type, Depth, User-Agent, X-File-Size, X-Requested-With, If-Modified-Since, X-File-Name, Cache-Control
Access-Control-Max-Age: 86400
Content-Type: text/xml;charset=UTF-8
Content-Length: 152
Proxy-Connection: Keep-Alive
Connection: Keep-Alive

<body xmlns='http://jabber.org/protocol/httpbind'><iq xmlns="jabber:client" type="result" id="purplef3ce4cf9" to="gorisis@xmpp.citnet/98a19540"/></body>

only the 1st reply (the empty <body/>) will be processed. Pidgin will not process the ping reply and this will lead to a time out.

Probably linked to bug #15679

Attachments (1)

pidgin.patch (1.7 KB) - added by flopma 3 years ago.
Patch to solve the described problem

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by flopma

Hi, I've submitted a patch to resolve this issue.

Can someone review it ?

Thx

comment:2 Changed 4 years ago by datallah

  • Milestone set to Patches Needing Review

comment:3 Changed 4 years ago by flopma

Hello,

Any chance to get this patch tested and a release made available for all ?

Thx

comment:4 Changed 4 years ago by datallah

  • Owner set to darkrain42

comment:5 Changed 3 years ago by MarkDoliner

Some feedback:

  • It's unfortunate that we have to g_strndup the body when printing it in purple_debug_info. It would be nice if we could first check if the log statement would even be written. Unfortunately this is really messy. It might be easier in main (if it's not easier then we should make it easier). In 2.x.y we'd have to do something like this, but it's really ugly and not worth it:
PurpleDebugUiOps *ops
ops = purple_debug_get_ui_ops();
if (purple_debug_is_enabled() || (ops
     && ops->print
     && (!ops->is_enabled || ops->is_enabled(PURPLE_DEBUG_INFO, "jabber")))) {
  message = g_strndup(data, len); 
  purple_debug_info(stuff); 
  g_free(message); 
}
  • Shifting the latter responses to the beginning of conn->read_buf isn't a great solution. It would be much better to leave the bytes where they are and just start parsing in the middle. But this is fine, since this is 2.x.y and we're getting rid of the http handling here in main.
  • Most importantly, does something actually call jabber_bosh_http_connection_process() again? Maybe it should be called in a while loop?

comment:6 Changed 3 years ago by flopma

Thx for your time.

Concerning g_strndup usage - as I don't know the core of pidgin, I can't answer. What I wanted to solve by adding this change was to only print the portion of char* data of length len. Unfortunately, the current version will print data completely while it might be longer than len.

Ok for your point 2, but I did not want to make bigger change in the pointer logic management (if we go your route).

Point 3 - you're right!

Changed 3 years ago by flopma

Patch to solve the described problem

comment:7 Changed 3 years ago by MarkDoliner

Cool, thanks for making that change. FYI we'll probably be releasing 2.10.8 in the next few weeks. I think it's very likely one of us will commit this patch before then.

comment:8 Changed 3 years ago by MarkDoliner

I committed this (with a few small changes--hopefully it still works) to our private repo as 5b9619d1dec4. This will get released in 2.10.8 in a week or two.

Do you have a name that we can use for you in our ChangeLog and COPYRIGHT file? (Or you can agree to grant copyright to Instant Messaging Freedom, Inc, our non-profit organization that holds the copyright for some of our code).

comment:9 Changed 3 years ago by MarkDoliner

  • Resolution set to fixed
  • Status changed from new to closed

comment:10 Changed 3 years ago by Robby

  • Milestone changed from Patches Needing Review to 2.10.8

comment:11 Changed 3 years ago by flopma

Hi, thx for inclusion of this patch. My name is Issa Gorissen (for changelog)

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!