Closed Bug 675983 Opened 11 years ago Closed 11 years ago

Websockets - fail connection on 1st fragment with a continuation op code


(Core :: Networking: WebSockets, defect)

Not set





(Reporter: mcmanus, Assigned: mcmanus)


(Blocks 1 open bug)



(1 file, 1 obsolete file) test cases 3.8 3.9. and 3.14 send a frame with a continuation op-code (and the fin bit set) that isn't part of a continuation sequence. (i.e. there is no previous first-frag which would have no fin bit and a non-continuation non-control op code).

This isn't part of a valid fragment sequence, and it isn't a valid unfragmented message either. We ignore it.

Draft -10 doesn't appear to have a specific handling for this, but we close the connection on other invalid protocol sequences so I think that's the right thing to do here too as suggested in 664344.
Blocks: 664344
Attached patch patch 1 (obsolete) — Splinter Review
Assignee: nobody → mcmanus
Attachment #550113 - Flags: review?(jduell.mcbugs)
+1 for closing on those invalid sequences, even when draft-10 doesn't specify that. I think it should.

One remark: with case 3.14, FF does ignore only the payload of the first "continuation that doesn't continue anything":

Message send: ["sendframes",[{"opcode":0,"fin":true,"payload":"fragment1"},{"opcode":1,"fin":false,"payload":"fragment2"},{"opcode":0,"fin":true,"payload":"fragment3"},{"opcode":0,"fin":true,"payload":"fragment1"},{"opcode":1,"fin":false,"payload":"fragment2"},{"opcode":0,"fin":true,"payload":"fragment3"}]]
Message received: fragment2fragment3
Message received: fragment1
Message received: fragment2fragment3

The payload "fragment1" (of the 2nd invalid frame) _is_ popped up in JS.

Btw: depressing to watch you fix those bugs much faster than it takes me to find them;)
(In reply to comment #2)

> One remark: with case 3.14, FF does ignore only the payload of the first
> "continuation that doesn't continue anything":

I think this patch will fix that.. we weren't really totally ignoring the bogus frame - it just seemed that way ;) It still impacted the state.

> Btw: depressing to watch you fix those bugs much faster than it takes me to
> find them;)

your tool is a great help to firefox - I really appreciate it!

When I get to the end of your list of things I will start a build for you with all the queued (but unreviewed and uncommitted) patches in it so you can retest. What platform do you use?
that'd be great! most of the time I'm on Win64 ..
(In reply to comment #4)
> that'd be great! most of the time I'm on Win64 ..

The build directory is here - the windows build will be done in 3 or 4 hours from now:
Comment on attachment 550113 [details] [diff] [review]
patch 1

Review of attachment 550113 [details] [diff] [review]:

Logic OK, but I'm submitting a cleaned up version.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +578,5 @@
>    mMaxMessageSize(16000000),
>    mStopOnClose(NS_OK),
>    mServerCloseCode(CLOSE_ABNORMAL),
>    mScriptCloseCode(0),
> +  mFragmentOpcode(kContinuation),

Is there any reason we store the fragment code in a PRUint8, when the spec dictates it's only 4 bits, so would fit in an unsigned char?  Or conversely, if we're going to keep it as an 8 bit value, why not initialize mFragmentOpcode to some "illegal" value, just for clarity? (I'm not doing either change in my patch: I'd lean toward using the 4-bit type: thoughts?).

Also, since we only set mFragmentOpCode once (for the first fragment), it's really the "overall message" opcode (and doesn't correspond to the current fragment's opcode).  I've renamed it mMessageType.

@@ +879,5 @@
>        if (opcode == kContinuation) {
> +
> +        // Make sure this continuation fragment isn't the first fragment
> +        if (mFragmentOpcode == kContinuation) {
> +          LOG(("WebSocketHeandler:: continuation code in first fragment\n"));

The wrong class name AND it's misspelled: for shame! :)

@@ +903,5 @@
>          payloadLength += mFragmentAccumulator;
>          avail += mFragmentAccumulator;
>          mFragmentAccumulator = 0;
>          opcode = mFragmentOpcode;
> +        mFragmentOpcode = kContinuation;


  // reset opcode to detect if next message illegally starts with continuation
Attachment #550113 - Flags: review?(jduell.mcbugs)
Let me know if you're ok with my fixes (and maybe change mMessageType to 4 bit if you think that's ok).
Attachment #550113 - Attachment is obsolete: true
Attachment #550974 - Flags: review+
Thanks for the updates jason. But actually, I prefer my patch in all respects except for the typo in the log message and your additional comment.

mMessageType is not correctly named. It is the opcode of an in-progress fragmented message - it does not apply to all messages.  mFragmentMessageOpcode would be acceptable to me if you insist on a change.

You also changed my log about nested fragments making it less informative imo (and shamefully left my mangled class name in it after you changed it!). That's not germane to this bug in any event.
I have mixed feelings on the 4 bit data type (you incur a lot of under the covers masking for no particular benefit.. there can only be 200 of these objects after all) - no reason to make it part of this bug fix in any event.

Scrapped my changes other than comment fixups.

Started to write test for this and other fragment msg cases, but ran into some stupid issues (python--meh), so moved to bug 677461.
Blocks: 677461
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.