Bug 80103 - [WebSocket] Should raise SYNTAX_ERR when message contains unpaired surrogates
Summary: [WebSocket] Should raise SYNTAX_ERR when message contains unpaired surrogates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
: 69405 (view as bug list)
Depends on: 80356 80385
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-03-01 22:33 PST by Kenichi Ishibashi
Modified: 2012-03-07 01:42 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.70 KB, patch)
2012-03-01 22:39 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (28.18 KB, patch)
2012-03-04 20:24 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch(Attempt QT build fix) (28.33 KB, patch)
2012-03-04 20:57 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (30.12 KB, patch)
2012-03-05 16:08 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (11.25 KB, patch)
2012-03-05 18:29 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (11.55 KB, patch)
2012-03-06 22:11 PST, Kenichi Ishibashi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-03-01 22:33:40 PST
See Chromium bug entry: http://crbug.com/116408
WebSocket message and close reason should not contain unpaired surrogates.
Comment 1 Kenichi Ishibashi 2012-03-01 22:39:23 PST
Created attachment 129824 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-03-01 22:42:23 PST
Hi Kent-san, Alexey,

Could you take a look?
Comment 3 Kenichi Ishibashi 2012-03-01 22:47:05 PST
Comment on attachment 129824 [details]
Patch

https://www.w3.org/Bugs/Public/show_bug.cgi?id=16157

There is a discussion for this check. I'll make consensus before review.
Comment 4 Kenichi Ishibashi 2012-03-01 22:47:50 PST
(In reply to comment #3)
> There is a discussion for this check. I'll make consensus before review.

s/I'll make/I'd like to make/
Comment 5 WebKit Review Bot 2012-03-02 01:36:31 PST
Comment on attachment 129824 [details]
Patch

Attachment 129824 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11779859

New failing tests:
http/tests/websocket/tests/hybi/unpaired-surrogates-in-message.html
Comment 6 Takashi Toyoshima 2012-03-02 10:12:22 PST
Comment on attachment 129824 [details]
Patch

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

Current spec say that we must raise SYNTAX_ERROR.
So fixing this issue sounds good.

I refer the discussion in order to justify that I think this is low priority issue.

Please request a review of Kent-san.

> Source/WebCore/Modules/websockets/WebSocket.cpp:113
> +        if (U16_IS_LEAD(message[index])) {

If you increment 'index' at first, following operation '- 1' and '+ 1' could be removed :-)
Comment 7 Yuta Kitamura 2012-03-02 11:00:50 PST
Comment on attachment 129824 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:108
> +    // FIXME: Any other errors except for unpaired surrogates?

I think String::utf8(true) will do the trick, including checking unpaired surrogates. If we do this, however, we convert the same string twice in both WebSocket and WebSocketChannel, which may affect the performance.

There will probably be two options:
(a) Let WebSocket convert the message into CString and pass it to WebSocketChannel.
(b) Let WebSocketChannel convert and validate a String, and return a value indicating a conversion error upon failure.

(a) sounds a bit strange, because WebSocketChannel is responsible for converting Blob and ArrayBuffer into CString. However, WebSocketChannel already accepts send(const char*, int) for PPAPI implementation purposes, so this might be okay.

(b) looks cleaner, but imposes an extra roundtrip between loader and worker if that WebSocket is on workers. The cost should be slight and negligible, though.

I think (b) would be better, but a patch for (b) will look hairy due to boilerplates of loader-worker bridge.
Comment 8 Kenichi Ishibashi 2012-03-04 20:24:20 PST
Created attachment 130050 [details]
Patch
Comment 9 Kenichi Ishibashi 2012-03-04 20:32:23 PST
Comment on attachment 129824 [details]
Patch

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

Thank you for comments!

>> Source/WebCore/Modules/websockets/WebSocket.cpp:108
>> +    // FIXME: Any other errors except for unpaired surrogates?
> 
> I think String::utf8(true) will do the trick, including checking unpaired surrogates. If we do this, however, we convert the same string twice in both WebSocket and WebSocketChannel, which may affect the performance.
> 
> There will probably be two options:
> (a) Let WebSocket convert the message into CString and pass it to WebSocketChannel.
> (b) Let WebSocketChannel convert and validate a String, and return a value indicating a conversion error upon failure.
> 
> (a) sounds a bit strange, because WebSocketChannel is responsible for converting Blob and ArrayBuffer into CString. However, WebSocketChannel already accepts send(const char*, int) for PPAPI implementation purposes, so this might be okay.
> 
> (b) looks cleaner, but imposes an extra roundtrip between loader and worker if that WebSocket is on workers. The cost should be slight and negligible, though.
> 
> I think (b) would be better, but a patch for (b) will look hairy due to boilerplates of loader-worker bridge.

Took (b) for send(). As for close(), there is already a utf8() call in the close() so I just changed it to be strict. I think it unlikely affects the performance because the valid length of the reason is limited.

>> Source/WebCore/Modules/websockets/WebSocket.cpp:113
>> +        if (U16_IS_LEAD(message[index])) {
> 
> If you increment 'index' at first, following operation '- 1' and '+ 1' could be removed :-)

Good catch:)
Comment 10 Early Warning System Bot 2012-03-04 20:32:53 PST
Comment on attachment 130050 [details]
Patch

Attachment 130050 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11801776
Comment 11 Kenichi Ishibashi 2012-03-04 20:57:34 PST
Created attachment 130053 [details]
Patch(Attempt QT build fix)
Comment 12 Yuta Kitamura 2012-03-05 11:19:36 PST
Comment on attachment 130053 [details]
Patch(Attempt QT build fix)

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

Looks good to me.

> Source/WebCore/Modules/websockets/WebSocket.cpp:356
> +        if (utf8.length() > maxReasonSizeInBytes) {
> +            ec = SYNTAX_ERR;
> +            return;
> +        }

Could you add a console message in this case as well?

optional: I suggest putting utf8.length() after utf8.isNull() located below.
Comment 13 Kenichi Ishibashi 2012-03-05 16:08:14 PST
Created attachment 130220 [details]
Patch
Comment 14 Kenichi Ishibashi 2012-03-05 16:08:57 PST
Comment on attachment 130053 [details]
Patch(Attempt QT build fix)

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

Yuta-san,

Thank you for review!

>> Source/WebCore/Modules/websockets/WebSocket.cpp:356
>> +        }
> 
> Could you add a console message in this case as well?
> 
> optional: I suggest putting utf8.length() after utf8.isNull() located below.

Done.
Comment 15 Kenichi Ishibashi 2012-03-05 16:09:38 PST
(In reply to comment #13)
> Created an attachment (id=130220) [details]
> Patch

Kent-san, could you review the patch?
Comment 16 Kent Tamura 2012-03-05 16:50:30 PST
Comment on attachment 130220 [details]
Patch

The approach is ok.  But would you separate this into two please?
1. Introduce ThreadableWebSocketChannel::SendResult
2. Add surrogate pair checking
Comment 17 Kenichi Ishibashi 2012-03-05 17:18:20 PST
(In reply to comment #16)
> (From update of attachment 130220 [details])
> The approach is ok.  But would you separate this into two please?
> 1. Introduce ThreadableWebSocketChannel::SendResult
> 2. Add surrogate pair checking

Sure. I uploaded a patch for (1) as bug 80356
Comment 18 Kenichi Ishibashi 2012-03-05 18:29:53 PST
Created attachment 130260 [details]
Patch
Comment 19 Kent Tamura 2012-03-05 18:32:20 PST
Comment on attachment 130260 [details]
Patch

ok
Comment 20 Kenichi Ishibashi 2012-03-05 18:33:13 PST
Comment on attachment 130260 [details]
Patch

Thank you!
Comment 21 WebKit Review Bot 2012-03-05 20:21:03 PST
Comment on attachment 130260 [details]
Patch

Clearing flags on attachment: 130260

Committed r109840: <http://trac.webkit.org/changeset/109840>
Comment 22 WebKit Review Bot 2012-03-05 20:21:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Kenichi Ishibashi 2012-03-05 20:23:50 PST
*** Bug 69405 has been marked as a duplicate of this bug. ***
Comment 24 Csaba Osztrogonác 2012-03-06 02:06:23 PST
(In reply to comment #21)
> (From update of attachment 130260 [details])
> Clearing flags on attachment: 130260
> 
> Committed r109840: <http://trac.webkit.org/changeset/109840>

The new tests fail on Qt with the following strange diffs:
--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-close-reason-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-close-reason-actual.txt 
@@ -4,7 +4,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 Connected.
-PASS ws.close(1000, 'í '); threw exception Error: SYNTAX_ERR: DOM Exception 12.
+PASS ws.close(1000, '); threw exception Error: SYNTAX_ERR: DOM Exception 12.
 PASS successfullyParsed is true
 
 TEST COMPLETE


--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-message-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-message-actual.txt 
@@ -4,7 +4,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 Connected.
-PASS ws.send('í '); threw exception Error: SYNTAX_ERR: DOM Exception 12.
+PASS ws.send('); threw exception Error: SYNTAX_ERR: DOM Exception 12.
 PASS successfullyParsed is true
 
 TEST COMPLETE


They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong?
Comment 25 Kent Tamura 2012-03-06 02:12:59 PST
(In reply to comment #24)
> -PASS ws.send('í '); threw exception Error: SYNTAX_ERR: DOM Exception 12.
> +PASS ws.send('); threw exception Error: SYNTAX_ERR: DOM Exception 12.
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE
> 
> 
> They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong?

They look similar to https://bugs.webkit.org/show_bug.cgi?id=78541#c8
Comment 26 Csaba Osztrogonác 2012-03-06 02:18:04 PST
(In reply to comment #25)

> They look similar to https://bugs.webkit.org/show_bug.cgi?id=78541#c8

Similar, but not same. In bug78451 the character is different, but here the character and the closing apostrophe is absolutely missing.
Comment 27 Kenichi Ishibashi 2012-03-06 03:41:30 PST
(In reply to comment #24)
 
> They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong?

I'm not sure where the difference came from, but the original string is an invalid UTF8 character(an unpaired surogate) so the string is essentially broken. I think the root cause is the same of https://bugs.webkit.org/show_bug.cgi?id=78541#c8. Looks like the tests were essentially passed.
Comment 28 Stephen White 2012-03-06 11:51:02 PST
Reverted r109840 for reason:

Broke WebSocket tests on Chrome Mac and Linux

Committed r109941: <http://trac.webkit.org/changeset/109941>
Comment 29 Stephen White 2012-03-06 11:56:44 PST
Sorry, this change didn't actually break the tests, but it was causing conflicts with the rollout of the one that did (https://bugs.webkit.org/show_bug.cgi?id=80356) so I rolled them both out.
Comment 30 Kenichi Ishibashi 2012-03-06 22:11:11 PST
Created attachment 130542 [details]
Patch for landing
Comment 31 Kenichi Ishibashi 2012-03-06 22:13:13 PST
(In reply to comment #30)
> Created an attachment (id=130542) [details]
> Patch for landing

Minor change in tests. Don't use shouldThrow() to workaround QT failure.
Comment 32 WebKit Review Bot 2012-03-07 01:09:55 PST
Comment on attachment 130542 [details]
Patch for landing

Rejecting attachment 130542 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11832870
Comment 33 Kenichi Ishibashi 2012-03-07 01:42:04 PST
Committed r110037: <http://trac.webkit.org/changeset/110037>