[WebSocket]Reserved1 bit should be 0 when no negotiated extension

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Li Yin]

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_.

[Li Yin]

From the websocket test site:
http://autobahn.ws/testsuite/reports/clients/index.html
Case 3.4 failed.

[Li Yin]

Created attachment 133201 [details]
Patch

[Benjamin Poulain]

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.

[Li Yin]

(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.

[Li Yin]

Created attachment 133402 [details]
Patch

[Benjamin Poulain]

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_.)

[Li Yin]

(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.

[Li Yin]

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.

[Benjamin Poulain]

(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.

[Li Yin]

(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.

[Li Yin]

Comment on attachment 133402 [details]
Patch

This bug has been fixed in the newest code.

[Li Yin]

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?

[Benjamin Poulain]

(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.