Bug 81878

Summary: [WebSocket]Reserved1 bit should be 0 when no negotiated extension
Product: WebKit Reporter: Li Yin <li.yin@intel.com>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth@webkit.org, ap@webkit.org, benjamin@webkit.org, tkent@chromium.org, yutak@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch li.yin: review-

Description From 2012-03-22 00:34:05 PST
RFC 6455
http://tools.ietf.org/html/rfc6455#section-5.2

MUST be 0 unless an extension is negotiated that defines meanings
for non-zero values.  If a nonzero value is received and none of
the negotiated extensions defines the meaning of such a nonzero
value, the receiving endpoint MUST _Fail the WebSocket Connection_.
------- Comment #1 From 2012-03-22 00:44:26 PST -------
From the websocket test site:
http://autobahn.ws/testsuite/reports/clients/index.html
Case 3.4 failed.
------- Comment #2 From 2012-03-22 00:51:03 PST -------
Created an attachment (id=133201) [details]
Patch
------- Comment #3 From 2012-03-22 17:15:07 PST -------
(From update of attachment 133201 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=133201&action=review

> Source/WebCore/ChangeLog:7
> +        [WebSocket]Reserved1 bit should be 0 when no negotiated extension
> +        https://bugs.webkit.org/show_bug.cgi?id=81878
> +
> +        Reviewed by NOBODY (OOPS!).
> +

I think it is worth adding a short explanation in this bug, linking the relevant spec.

> Source/WebCore/ChangeLog:8
> +        No new tests, because it was covered by reserved-bits.html

This is not right.
If it was covered, why did you have to modify the test?

> LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:49
> -doTest(2);
> +doTest(1);

This seems bogus, you are changing the coverage of an existing test.

The function doTest() also has a comment made false with this patch: "// bitNumber must be 2 or 3.".

> LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:7
> -    pass
> +    request.ws_extension_processors = []

This is most mysterious. An explanation in the ChangeLog would be welcome.
------- Comment #4 From 2012-03-22 17:46:11 PST -------
(In reply to comment #3)
> (From update of attachment 133201 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133201&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        [WebSocket]Reserved1 bit should be 0 when no negotiated extension
> > +        https://bugs.webkit.org/show_bug.cgi?id=81878
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> I think it is worth adding a short explanation in this bug, linking the relevant spec.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests, because it was covered by reserved-bits.html
> 
> This is not right.
> If it was covered, why did you have to modify the test?
> 
> > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:49
> > -doTest(2);
> > +doTest(1);
> 
> This seems bogus, you are changing the coverage of an existing test.
> 
> The function doTest() also has a comment made false with this patch: "// bitNumber must be 2 or 3.".
> 
> > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:7
> > -    pass
> > +    request.ws_extension_processors = []
> 
> This is most mysterious. An explanation in the ChangeLog would be welcome.

Thanks for your review.
Update the patch. Please have a check.
------- Comment #5 From 2012-03-22 17:48:39 PST -------
Created an attachment (id=133402) [details]
Patch
------- Comment #6 From 2012-03-22 17:57:21 PST -------
(From update of attachment 133402 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=133402&action=review

> Source/WebCore/ChangeLog:12
> +        RFC 6455 http://tools.ietf.org/html/rfc6455#section-5.2
> +        MUST be 0 unless an extension is negotiated that defines meanings
> +        for non-zero values. If a nonzero value is received and none of
> +        the negotiated extensions defines the meaning of such a nonzero
> +        value, the receiving endpoint MUST _Fail the WebSocket Connection_.

Now that I see a bit more what this is about...

Is there also a test in WebKit for the positive case? Setting the bit number and having an extension.
What about setting a bit, having an extension, but the extension does not define the meaning of the bit? (per: If a nonzero value is received and none of the negotiated extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocketConnection_.)
------- Comment #7 From 2012-03-22 18:21:02 PST -------
(In reply to comment #6)
> (From update of attachment 133402 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133402&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        RFC 6455 http://tools.ietf.org/html/rfc6455#section-5.2
> > +        MUST be 0 unless an extension is negotiated that defines meanings
> > +        for non-zero values. If a nonzero value is received and none of
> > +        the negotiated extensions defines the meaning of such a nonzero
> > +        value, the receiving endpoint MUST _Fail the WebSocket Connection_.
> 
> Now that I see a bit more what this is about...
> 
> Is there also a test in WebKit for the positive case? Setting the bit number and having an extension.
Yes, the normal test based on this way, for example simple.html.

> What about setting a bit, having an extension, but the extension does not define the meaning of the bit? (per: If a nonzero value is received and none of the negotiated extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocketConnection_.)
If it has negotiated extension in WebKit, the reserved bit1 means compress bit. If the reserved1 bit is set to be 1, it means the frame is compressed, or the frame is uncompressed. WebKit can handle these frames successfully.
But it seems no test case to cover it.
------- Comment #8 From 2012-03-22 18:25:57 PST -------
Hi benjamin,
   Do you think it is necessary to design a new test to cover the following case:
   It has negotiated extension, but the reserved1 bit is set to be 0.
------- Comment #9 From 2012-03-22 18:31:04 PST -------
(In reply to comment #8)
>    Do you think it is necessary to design a new test to cover the following case:
>    It has negotiated extension, but the reserved1 bit is set to be 0.

Yep, that would be neat.
This is directly related to this patch, it is all about making sure we correctly check in input of the Reserved bits.
------- Comment #10 From 2012-03-22 22:01:51 PST -------
(In reply to comment #9)
> (In reply to comment #8)
> >    Do you think it is necessary to design a new test to cover the following case:
> >    It has negotiated extension, but the reserved1 bit is set to be 0.
> 
> Yep, that would be neat.
> This is directly related to this patch, it is all about making sure we correctly check in input of the Reserved bits.

Sorry for my misleading, according to the investigation, this kind of case has been covered by current test, such as zero-length-text.html, receive-blob.html and so on, most of the former test cases based on uncompressed frames. 
So, maybe it's not necessary to design the duplicate case.
------- Comment #11 From 2012-03-23 07:09:44 PST -------
(From update of attachment 133402 [details])
This bug has been fixed in the newest code.
------- Comment #12 From 2012-03-23 07:20:31 PST -------
Maybe the test case should be redesigned, it had better cover both no-extension and extension scenarios.

extension bit1 bit2 bit3
   yes     0    1    0
   yes     0    0    1
   no      1    0    0
   no      0    1    0
   no      0    0    1

The current test case only covered the 1st and 2nd.
But the 3rd, 4th and 5th were not covered. 

What's your opinion?
------- Comment #13 From 2012-03-23 12:15:34 PST -------
(In reply to comment #12)
> Maybe the test case should be redesigned, it had better cover both no-extension and extension scenarios.
> 
> extension bit1 bit2 bit3
>    yes     0    1    0
>    yes     0    0    1
>    no      1    0    0
>    no      0    1    0
>    no      0    0    1
> 
> The current test case only covered the 1st and 2nd.
> But the 3rd, 4th and 5th were not covered. 
> 
> What's your opinion?

Improving the test coverage sounds good. You should submit the test separately and CC the engineer who made the fix you mentioned.