Race condition causes sender to send incorrect frame header. about ws HOT 5 CLOSED

Awesome bug report :) That makes it an easy fix for me. Thanks a bunch!

On Feb 29, 2012 4:17 AM, "Chris Shoemaker" <
[email protected]>
wrote:

It has been very difficult to narrow this bug down, but it is 100%
reproducible. To reproduce:

1. The server needs to be sending messages faster than the client can
   receive them. I need to generate at least 100 messages from server to
   client, but 200 is more reliable.
2. The length of the messages needs to be very consistent but gradually
   increase. This is so you can observe a long sequence of identically
   repeating frame headers. I accomplished this by embedding a counter
   integer in the message, so when the counter was less than 9, the integer
   serialized into one byte; when it was between 10 and 99, it required two
   bytes, etc. For example, my first 9 messages were each 207 bytes long,
   then messages 10-99 were each 208 bytes long, then messages 100+ were (or
   would be) each 209 bytes long ( which would only be correct for a message
   with counter > 99 ).
3. What you can observe, either through packet sniffing or by
   instrumenting the client, is that the frame headers received by the client
   show a frame length of 207 for 9 message, then 208 for messages 10-99, etc.
   At least, that's what you should see. What actually happens is that,
   under the right conditions, the receiver will consistently receive an frame
   length that corresponds to a later message along with the message body for
   the current body. In my example, somewhere between message 70-73 (that is,
   still with a two digit counter in the message body) the sender will use the
   message length of 209 bytes.
4. The problem is here:
   https://github.com/einaros/ws/blob/06121f89f5ab52a242717f68e4d9dc36fe771f2a/lib/Sender.js#L126
   The problem goes completely away if I force this line to always create a
   new Buffer(totalLength).
5. If I inspect the contents of the outputBuffer just before the call to
   Socket.send, (
   https://github.com/einaros/ws/blob/06121f89f5ab52a242717f68e4d9dc36fe771f2a/lib/Sender.js#L172)
   I see correct values, but when the bytes actually hit the wire (as
   measured using wireshark) the incorrect value is sent.
6. The eventual symptom in my case was a "invalid utf8 sequence" error,
   because the message length was one byte too high, so it would read the
   first byte (129 = finalFragment | opcode) of the next frame and try to
   validate it as utf8.
7. Of course, there's a one-liner fix that works, but avoids the use of
   the _sendCache Buffer. How important is this cache? What are the
   synchronization mechanisms for avoiding this bug?

Note: I cannot reproduce this using a single machine. I have to use a
remote server.
Note2: The actual trigger points are sensitive to the message lengths.
With shorter messages, I can get it to fail around message 80-85, and with
longer messages, I can get it to fail around message 60-65.

node version: 0.6.10
ws version: 0.4.7
arch: x86_64 linux

---

Reply to this email directly or view it on GitHub:
#35

from ws.

Could you try 0.4.8 and see if it still reproduces?

from ws.

I confirm. Bug fixed in 0.4.8.

from ws.

thanks einaros

from ws.

I had been pondering exactly why I added this lately. At some point (perhaps earlier in 0.6) I did see a perf gain from running it. With 0.6.11 and a newly written throughput benchmark, no perf difference could be found at all.

from ws.