WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(5.03 KB, patch)
2012-03-17 03:39 PDT
,
Li Yin
no flags
Details
Formatted Diff
Diff
Patch
(5.90 KB, patch)
2012-03-17 18:39 PDT
,
Li Yin
no flags
Details
Formatted Diff
Diff
Patch
(7.49 KB, patch)
2012-03-20 22:55 PDT
,
Li Yin
no flags
Details
Formatted Diff
Diff
Patch
(7.50 KB, patch)
2012-03-21 17:16 PDT
,
Li Yin
no flags
Details
Formatted Diff
Diff
Patch
(7.50 KB, patch)
2012-03-21 18:26 PDT
,
Li Yin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Li Yin
Comment 1
2012-03-16 19:34:59 PDT
Created
attachment 132437
[details]
Patch
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
Created
attachment 132456
[details]
Patch
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
Created
attachment 132476
[details]
Patch
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
Created
attachment 132976
[details]
Patch
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
Created
attachment 133147
[details]
Patch
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
Created
attachment 133162
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug