Bug 81878 - [WebSocket]Reserved1 bit should be 0 when no negotiated extension
: [WebSocket]Reserved1 bit should be 0 when no negotiated extension
Status: RESOLVED INVALID
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-22 00:34 PDT by Li Yin
Modified: 2012-03-24 19:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2012-03-22 00:51 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2012-03-22 17:48 PDT, Li Yin
li.yin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-03-22 00:34:05 PDT
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 Li Yin 2012-03-22 00:44:26 PDT
From the websocket test site:
http://autobahn.ws/testsuite/reports/clients/index.html
Case 3.4 failed.
Comment 2 Li Yin 2012-03-22 00:51:03 PDT
Created attachment 133201 [details]
Patch
Comment 3 Benjamin Poulain 2012-03-22 17:15:07 PDT
Comment on attachment 133201 [details]
Patch

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 Li Yin 2012-03-22 17:46:11 PDT
(In reply to comment #3)
> (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.

Thanks for your review.
Update the patch. Please have a check.
Comment 5 Li Yin 2012-03-22 17:48:39 PDT
Created attachment 133402 [details]
Patch
Comment 6 Benjamin Poulain 2012-03-22 17:57:21 PDT
Comment on attachment 133402 [details]
Patch

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 Li Yin 2012-03-22 18:21:02 PDT
(In reply to comment #6)
> (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.
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 Li Yin 2012-03-22 18:25:57 PDT
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 Benjamin Poulain 2012-03-22 18:31:04 PDT
(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 Li Yin 2012-03-22 22:01:51 PDT
(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 Li Yin 2012-03-23 07:09:44 PDT
Comment on attachment 133402 [details]
Patch

This bug has been fixed in the newest code.
Comment 12 Li Yin 2012-03-23 07:20:31 PDT
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 Benjamin Poulain 2012-03-23 12:15:34 PDT
(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.