search

xmppo / node-xmpp-bosh

An XMPP BOSH & WebSocket server (connection manager) written on node.js using Javascript
https://github.com/xmppo/node-xmpp-bosh
263 stars 85 forks source link

Config. variable max_xmpp_buffer_size not respected in current HEAD #23

Closed dhruvbird closed 12 years ago

dhruvbird commented 12 years ago

Probably _on_data() & _on_stanza() should update a counter and maintain the buffer under max_xmpp_buffer_size.

satyamshekhar commented 12 years ago

Done. This actually limits the xmpp stanza size. We should also put in a limit for the buffer size - size of pending_stanzas.

dhruvbird commented 12 years ago

Thanks :)

On Tue, Apr 3, 2012 at 1:47 AM, satyamshekhar reply@reply.github.com wrote:

Done. This actually limits the xmpp stanza size. We should also put in a limit for the buffer size - size of pending_stanzas.

Reply to this email directly or view it on GitHub: https://github.com/dhruvbird/node-xmpp-bosh/issues/23#issuecomment-4894934

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

dhruvbird commented 12 years ago

Just wondering - suppose the limit on the packet size is 100 bytes and the server sends 200 bytes in one go, won't that trigger this check? Post-parsing though, each stanza might actually be <= 100 bytes in size?

On Tue, Apr 3, 2012 at 9:16 AM, Dhruv Matani dhruvbird@gmail.com wrote:

Thanks :)

On Tue, Apr 3, 2012 at 1:47 AM, satyamshekhar reply@reply.github.com wrote:

Done. This actually limits the xmpp stanza size. We should also put in a limit for the buffer size - size of pending_stanzas.

Reply to this email directly or view it on GitHub: https://github.com/dhruvbird/node-xmpp-bosh/issues/23#issuecomment-4894934

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

satyamshekhar commented 12 years ago

Yup, this check will not terminate that the stream in that case..

dhruvbird commented 12 years ago

I am guessing that the check you have implemented is for buffer size limit.

On Tue, Apr 3, 2012 at 2:56 PM, satyamshekhar reply@reply.github.com wrote:

Yup, this check will not terminate that the stream in that case..

Reply to this email directly or view it on GitHub: https://github.com/dhruvbird/node-xmpp-bosh/issues/23#issuecomment-4920931

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

satyamshekhar commented 12 years ago

No, this only checks the stanza size. _current_stanza_size is reset in function _on_stanza. To implement buffer size limit we will have to limit the size of pending_stanzas right?

dhruvbird commented 12 years ago

What I meant to say is that the current check does more than just the stanza size - it disallows valid stanzas < the limit if the server decided to push multiple stanzas at one go right?

So in essence, it is limiting the amount of buffer space we allocate for incoming packets for a certain stream.

On Tue, Apr 3, 2012 at 3:05 PM, satyamshekhar reply@reply.github.com wrote:

No, this only checks the stanza size. _current_stanza_size is reset in function _on_stanza. To implement buffer size limit we will have to limit the size of pending_stanzas right?

Reply to this email directly or view it on GitHub: https://github.com/dhruvbird/node-xmpp-bosh/issues/23#issuecomment-4921533

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

satyamshekhar commented 12 years ago

Oh, now I get what you are saying.. Actually, not necessarily.. this depends on the network/node.js.. it might emit the data into diff chunks.. The size of the chunks is not decided by the remote server, is it?

dhruvbird commented 12 years ago

On Tue, Apr 3, 2012 at 3:17 PM, satyamshekhar reply@reply.github.com wrote:

Oh, now I get what you are saying.. Actually, not necessarily.. this depends on the network/node.js.. it might emit the data into diff chunks.. The size of the chunks is not decided by the remote server, is it?

Exactly! So node itself might actually accumulate multiple packets and push them out right?

Actually, your last observation makes me thing that it is not entirely possible to reliably limit the stanza size (or the buffer size) unless it has been completely parsed or the last chunk of the stanza remains. Consider when the stanza limit is 100 bytes and we have received 90 bytes. The server (or node) then sends 100 bytes, 5 of which end the current stanza and the remaining start a new one. Our check fails here.

However, we can have a config variable which limits the amount of data we are willing to receive and hold and process on behalf of the server (this is essentially the buffer size). Again, this would be best effort.

Reply to this email directly or view it on GitHub: https://github.com/dhruvbird/node-xmpp-bosh/issues/23#issuecomment-4921946

   -Dhruv Matani. http://dhruvbird.com/

"What's the simplest thing that could possibly work?" -- Ward Cunningham

satyamshekhar commented 12 years ago

Exactly! So node itself might actually accumulate multiple packets and push them out right?

It can accumulate packets in case current tick takes time.. What we can do is move the check after we write the incoming data to the parser.. that will limit the stanza size right?

We can have another check that limits the chunk size.. Like you already suggested..

sonnyp commented 12 years ago

Sorry, wrong issue number :-/

dhruvbird commented 12 years ago

@satyamshekhar @astro has pushed a feature to node-expat that allows one to query the number of bytes read in the stream till now. I've tried to fix the stanza limit restriction. Please could you review it for correctness. Caveat: It is possible that stanzas > the limit are accepted at times, but never will stanzas <= the limit be rejected. The logic is somewhat like a bloom filter.

satyamshekhar commented 12 years ago

looks good. (y)