einaros/ws — Issue #35: Race condition causes sender to send incorrect frame header.
Author: shoe
Created: 2012-02-29 03:17:53+00:00
State: open

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