[WebSocket]The minimal number of bytes MUST be used to encode the length

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Li Yin]

From REF 6455:
http://tools.ietf.org/html/rfc6455#section-5.2
Note that in all cases, the minimal number of bytes MUST be used to encode
the length, for example, the length of a 124-byte-long string
can't be encoded as the sequence 126, 0, 124.

[Li Yin]

Created attachment 132437 [details]
Patch

[Benjamin Poulain]

Comment on attachment 132437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132437&action=review

> Source/WebCore/ChangeLog:10
> +        No new test.

Why no tests?

[Benjamin Poulain]

Comment on attachment 132437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132437&action=review

> Source/WebCore/ChangeLog:10
> +        No new test.

Why no tests?

[Li Yin]

Created attachment 132456 [details]
Patch

[Li Yin]

Add the test case, and update the code. 
Please have a check.

[Li Yin]

Does the test case need to be split another patch?

[Benjamin Poulain]

Comment on attachment 132456 [details]
Patch

(In reply to comment #6)
> Does the test case need to be split another patch?

The test should be submitted along the patch.

I r- because you need to update the ChangeLog. A ChangeLog should be created for the layout test.

I don't know WebSocket well enough to review this, I will leave the positive review for someone else.

[Li Yin]

Created attachment 132476 [details]
Patch

[Li Yin]

Add the Change Log in Layout test

[Li Yin]

Hi tkent,yuta,
  Can you review this patch? 
  Thanks.

[Yuta Kitamura]

Comment on attachment 132476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132476&action=review

The code looks okay, but the test can be improved.

> Source/WebCore/ChangeLog:8
> +        [WebSocket]The minimal number of bytes MUST be used to encode the length
> +        https://bugs.webkit.org/show_bug.cgi?id=81443
> +        From RFC 6455 http://tools.ietf.org/html/rfc6455#section-5.2
> +        the minimal number of bytes MUST be used to encode the length
> +
> +        Reviewed by NOBODY (OOPS!).

Our ChangeLog format is usually:

  <bug title>
  <bug URL>

  Reviewed by ...

  <more description>

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:12
> +    # pywebsocket refuses to create a frame with error encode length.
> +    # Thus, we need to build a frame manually.

You can use create_header() function from mod_pywebsocket.stream module to build a (possibly incorrect) frame header. See reserved-bits_wsh.py for example.

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:14
> +    header += chr(126) # No Mask and two bytes extended payload length.

It would be better if we could test the edge cases like
  a) payload len = 126, extended payload length = 125
  b) payload len = 127, extended payload length = 0xFFFF
  c) payload len = {126,127}, extended payload length = 0

To test multiple cases, you may want to make use of query parameters in the URL; see protocol-test_wsh.py or reserved-bits_wsh.py for example.

[Yuta Kitamura]

Comment on attachment 132476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132476&action=review

>> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:12
>> +    # Thus, we need to build a frame manually.
> 
> You can use create_header() function from mod_pywebsocket.stream module to build a (possibly incorrect) frame header. See reserved-bits_wsh.py for example.

Well, I was wrong, and you cannot use create_header() in this case. It's probably okay to build a frame header manually.

[Li Yin]

Created attachment 132976 [details]
Patch

[Li Yin]

(In reply to comment #11)
> (From update of attachment 132476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132476&action=review
> 
> The code looks okay, but the test can be improved.
> 
> > Source/WebCore/ChangeLog:8
> > +        [WebSocket]The minimal number of bytes MUST be used to encode the length
> > +        https://bugs.webkit.org/show_bug.cgi?id=81443
> > +        From RFC 6455 http://tools.ietf.org/html/rfc6455#section-5.2
> > +        the minimal number of bytes MUST be used to encode the length
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Our ChangeLog format is usually:
> 
>   <bug title>
>   <bug URL>
> 
>   Reviewed by ...
> 
>   <more description>
> 
> > LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:12
> > +    # pywebsocket refuses to create a frame with error encode length.
> > +    # Thus, we need to build a frame manually.
> 
> You can use create_header() function from mod_pywebsocket.stream module to build a (possibly incorrect) frame header. See reserved-bits_wsh.py for example.
> 
> > LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:14
> > +    header += chr(126) # No Mask and two bytes extended payload length.
> 
> It would be better if we could test the edge cases like
>   a) payload len = 126, extended payload length = 125
>   b) payload len = 127, extended payload length = 0xFFFF
>   c) payload len = {126,127}, extended payload length = 0
> 
> To test multiple cases, you may want to make use of query parameters in the URL; see protocol-test_wsh.py or reserved-bits_wsh.py for example.

Thanks a million for your detailed review.

[Li Yin]

Update the test case covering four cases:
payload length:126, extended length:125
payload length:126, extended length:0
payload length:127, extended length:65535(0xFFFF)
payload length:127, extended length:0

[Yuta Kitamura]

Comment on attachment 132976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132976&action=review

I just have style comments. The code itself looks fine.

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:16
> +    payloadLength, extendedLength = (match.group(1)).split('_', 1)

Should be payload_length, extended_length respectively.

We generally follow PEP8 in python files, so please take a look at it.

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:18
> +    payloadLength = int (payloadLength)
> +    extendedLength = int (extendedLength)

Space after "int" is unnecessary.

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:24
> +    if payloadLength == 126 :

Space before colon is unnecessary. For here and next two as well.

[Li Yin]

Created attachment 133147 [details]
Patch

[Li Yin]

Hi Kent,
   Could you review this patch? 
   Thanks.

[Kent Tamura]

Comment on attachment 133147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133147&action=review

> LayoutTests/http/tests/websocket/tests/hybi/invalid-encode-length_wsh.py:28
> +    else :

nit: else : -> else:

[Li Yin]

Created attachment 133162 [details]
Patch

[Kent Tamura]

Comment on attachment 133162 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 133162 [details]
Patch

Clearing flags on attachment: 133162

Committed r111828: <http://trac.webkit.org/changeset/111828>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.