RESOLVED FIXED 81443
[WebSocket]The minimal number of bytes MUST be used to encode the length
https://bugs.webkit.org/show_bug.cgi?id=81443
Summary [WebSocket]The minimal number of bytes MUST be used to encode the length
Li Yin
Reported 2012-03-16 19:28:36 PDT
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.
Attachments
Patch (2.04 KB, patch)
2012-03-16 19:34 PDT, Li Yin
no flags
Patch (5.03 KB, patch)
2012-03-17 03:39 PDT, Li Yin
no flags
Patch (5.90 KB, patch)
2012-03-17 18:39 PDT, Li Yin
no flags
Patch (7.49 KB, patch)
2012-03-20 22:55 PDT, Li Yin
no flags
Patch (7.50 KB, patch)
2012-03-21 17:16 PDT, Li Yin
no flags
Patch (7.50 KB, patch)
2012-03-21 18:26 PDT, Li Yin
no flags
Li Yin
Comment 1 2012-03-16 19:34:59 PDT
Benjamin Poulain
Comment 2 2012-03-16 23:50:01 PDT
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 3 2012-03-16 23:50:01 PDT
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
Comment 4 2012-03-17 03:39:07 PDT
Li Yin
Comment 5 2012-03-17 03:42:16 PDT
Add the test case, and update the code. Please have a check.
Li Yin
Comment 6 2012-03-17 03:47:44 PDT
Does the test case need to be split another patch?
Benjamin Poulain
Comment 7 2012-03-17 13:57:05 PDT
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
Comment 8 2012-03-17 18:39:45 PDT
Li Yin
Comment 9 2012-03-17 18:42:33 PDT
Add the Change Log in Layout test
Li Yin
Comment 10 2012-03-19 17:28:07 PDT
Hi tkent,yuta, Can you review this patch? Thanks.
Yuta Kitamura
Comment 11 2012-03-20 10:50:32 PDT
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 12 2012-03-20 10:55:10 PDT
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
Comment 13 2012-03-20 22:55:55 PDT
Li Yin
Comment 14 2012-03-20 22:57:34 PDT
(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
Comment 15 2012-03-20 23:00:19 PDT
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 16 2012-03-21 11:19:55 PDT
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
Comment 17 2012-03-21 17:16:37 PDT
Li Yin
Comment 18 2012-03-21 17:18:26 PDT
Hi Kent, Could you review this patch? Thanks.
Kent Tamura
Comment 19 2012-03-21 18:19:27 PDT
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
Comment 20 2012-03-21 18:26:50 PDT
Kent Tamura
Comment 21 2012-03-21 18:28:38 PDT
Comment on attachment 133162 [details] Patch ok
WebKit Review Bot
Comment 22 2012-03-22 23:04:16 PDT
Comment on attachment 133162 [details] Patch Clearing flags on attachment: 133162 Committed r111828: <http://trac.webkit.org/changeset/111828>
WebKit Review Bot
Comment 23 2012-03-22 23:04:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.