Bug 80103 – [WebSocket] Should raise SYNTAX_ERR when message contains unpaired surrogates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kenichi Ishibashi]

See Chromium bug entry: http://crbug.com/116408
WebSocket message and close reason should not contain unpaired surrogates.

[Kenichi Ishibashi]

Created attachment 129824 [details]
Patch

[Kenichi Ishibashi]

Hi Kent-san, Alexey,

Could you take a look?

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

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

[WebKit Review Bot]

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

[Takashi Toyoshima]

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 :-)

[Yuta Kitamura]

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.

[Kenichi Ishibashi]

Created attachment 130050 [details]
Patch

[Kenichi Ishibashi]

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:)

[Early Warning System Bot]

Comment on attachment 130050 [details]
Patch

Attachment 130050 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11801776

[Kenichi Ishibashi]

Created attachment 130053 [details]
Patch(Attempt QT build fix)

[Yuta Kitamura]

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.

[Kenichi Ishibashi]

Created attachment 130220 [details]
Patch

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

(In reply to comment #13)
> Created an attachment (id=130220) [details]
> Patch

Kent-san, could you review the patch?

[Kent Tamura]

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

[Kenichi Ishibashi]

(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

[Kenichi Ishibashi]

Created attachment 130260 [details]
Patch

[Kent Tamura]

Comment on attachment 130260 [details]
Patch

ok

[Kenichi Ishibashi]

Comment on attachment 130260 [details]
Patch

Thank you!

[WebKit Review Bot]

Comment on attachment 130260 [details]
Patch

Clearing flags on attachment: 130260

Committed r109840: <http://trac.webkit.org/changeset/109840>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

*** Bug 69405 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

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

[Kent Tamura]

(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

[Csaba Osztrogonác]

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

[Kenichi Ishibashi]

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

[Stephen White]

Reverted r109840 for reason:

Broke WebSocket tests on Chrome Mac and Linux

Committed r109941: <http://trac.webkit.org/changeset/109941>

[Stephen White]

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.

[Kenichi Ishibashi]

Created attachment 130542 [details]
Patch for landing

[Kenichi Ishibashi]

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

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Committed r110037: <http://trac.webkit.org/changeset/110037>