Bug 64522

Created attachment 100803 [details] Patch

This patch is big, but most of the patch is occupied by newly added tests.

Not sure why style bot turned red. When I run check-webkit-style locally, it passes.

Comment on attachment 100803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100803&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:412 > +const WebSocketChannel::OpCode WebSocketChannel::OpCodeContinuation = 0x0; > +const WebSocketChannel::OpCode WebSocketChannel::OpCodeText = 0x1; > +const WebSocketChannel::OpCode WebSocketChannel::OpCodeBinary = 0x2; > +const WebSocketChannel::OpCode WebSocketChannel::OpCodeClose = 0x8; > +const WebSocketChannel::OpCode WebSocketChannel::OpCodePing = 0x9; > +const WebSocketChannel::OpCode WebSocketChannel::OpCodePong = 0xA; > + I think we had better move them to the upper place of this file. These symbols are used before here, and I'm afraid defining them here prevents constant value folding optimization. > Source/WebCore/websockets/WebSocketChannel.cpp:413 > +bool WebSocketChannel::processFrame() This function is too large. Please consider splitting it. > Source/WebCore/websockets/WebSocketChannel.cpp:417 > + char* p = m_buffer; > + char* end = m_buffer + m_bufferSize; 'p' and 'end' should be 'const char*'. Use another non-const variable or const_cast<> when we unmask the payload. 'end' should be 'bufferEnd' to avoid confusion with frameEnd'. > Source/WebCore/websockets/WebSocketChannel.cpp:420 > + if (m_bufferSize < 2) // Frame incomplete. > + return false; The comment should be a complete sentence as possible. Probably "The frame is incomplete." > Source/WebCore/websockets/WebSocketChannel.cpp:434 > + if (payloadLength64 >= 126) { > + int extendedPayloadLengthSize = payloadLength64 == 126 ? 2 : 8; 126 is a magic number. Please introduce a meaningful name. > Source/WebCore/websockets/WebSocketChannel.cpp:448 > +#if OS(WINDOWS) > + static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFui64; > +#else > + static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFull; > +#endif It's not OS-dependent but compiler-dependent. You assume uint64_t is available on any platforms. So can you use UINT64_C(0x7FFFFFFFFFFFFFFF) ? > Source/WebCore/websockets/WebSocketChannel.cpp:449 > + size_t maskingKeyLength = masked ? 4 : 0; 4 is a magic number. > Source/WebCore/websockets/WebSocketChannel.cpp:464 > + char* payload = p + maskingKeyLength; > + char* frameEnd = p + maskingKeyLength + payloadLength; They should be const char*. > Source/WebCore/websockets/WebSocketChannel.cpp:677 > + frame.append(0x80 | opCode); // 0x80 is for "fin" bit; we do not fragment a frame on sending. > + if (dataLength < 126) > + frame.append(0x80 | dataLength); // 0x80 is for "MASK" bit; a client must mask frames so it is always on. So we should have FinBit = 0x80 and MaskBit = 0x80, or something like them. > Source/WebCore/websockets/WebSocketChannel.cpp:703 > + frame.append("....", 4); // Four-byte placeholder for masking key. Will be overwritten. > + size_t payloadStart = frame.size(); > + frame.append(data, dataLength); > + > + cryptographicallyRandomValues(frame.data() + maskingKeyStart, 4); > + for (size_t i = 0; i < dataLength; ++i) > + frame[payloadStart + i] ^= frame[maskingKeyStart + i % 4]; 4 is a magic number.

Created attachment 100959 [details] Patch v2

Comment on attachment 100803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100803&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:412 >> + > > I think we had better move them to the upper place of this file. > These symbols are used before here, and I'm afraid defining them here prevents constant value folding optimization. Fixed. >> Source/WebCore/websockets/WebSocketChannel.cpp:413 >> +bool WebSocketChannel::processFrame() > > This function is too large. Please consider splitting it. The first half has been moved to a new function parseFrame(). >> Source/WebCore/websockets/WebSocketChannel.cpp:417 >> + char* end = m_buffer + m_bufferSize; > > 'p' and 'end' should be 'const char*'. > Use another non-const variable or const_cast<> when we unmask the payload. > > 'end' should be 'bufferEnd' to avoid confusion with frameEnd'. Fixed. >> Source/WebCore/websockets/WebSocketChannel.cpp:420 >> + return false; > > The comment should be a complete sentence as possible. Probably "The frame is incomplete." Removed this comment because this "return" statement has been changed to "return FrameIncomplete;" (FrameIncomplete is a member of a new enum ParseFrameResult) and the code looked self-documenting to me. >> Source/WebCore/websockets/WebSocketChannel.cpp:434 >> + int extendedPayloadLengthSize = payloadLength64 == 126 ? 2 : 8; > > 126 is a magic number. > Please introduce a meaningful name. Fixed. I gave names to 125, 126 and 127. >> Source/WebCore/websockets/WebSocketChannel.cpp:448 >> +#endif > > It's not OS-dependent but compiler-dependent. > > You assume uint64_t is available on any platforms. So can you use UINT64_C(0x7FFFFFFFFFFFFFFF) ? I didn't know UINT64_C, and it seems to work fine. Fixed. >> Source/WebCore/websockets/WebSocketChannel.cpp:449 >> + size_t maskingKeyLength = masked ? 4 : 0; > > 4 is a magic number. Created a new constant. >> Source/WebCore/websockets/WebSocketChannel.cpp:464 >> + char* frameEnd = p + maskingKeyLength + payloadLength; > > They should be const char*. Fixed. >> Source/WebCore/websockets/WebSocketChannel.cpp:677 >> + frame.append(0x80 | dataLength); // 0x80 is for "MASK" bit; a client must mask frames so it is always on. > > So we should have FinBit = 0x80 and MaskBit = 0x80, or something like them. Fixed. >> Source/WebCore/websockets/WebSocketChannel.cpp:703 >> + frame[payloadStart + i] ^= frame[maskingKeyStart + i % 4]; > > 4 is a magic number. Fixed.

Comment on attachment 100959 [details] Patch v2 Attachment 100959 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9095202

Oh no, Qt doesn't have UINT64_C...

(In reply to comment #8) > Oh no, Qt doesn't have UINT64_C... I'm not sure about how to resolve this. Maybe I should use "#if PLATFORM(QT)"?

(In reply to comment #9) > (In reply to comment #8) > > Oh no, Qt doesn't have UINT64_C... > > I'm not sure about how to resolve this. Maybe I should use "#if PLATFORM(QT)"? How about including stdint.h explicitly? BTW, I wonder whether UINT64_C is available on Chromium-win (VC++ 2008).

Created attachment 101269 [details] Patch v3

(In reply to comment #11) > Created an attachment (id=101269) [details] > Patch v3 Added #include <stdint.h>. I posted try jobs to Chromium tryserver to see if this patch works (they are not finished yet). http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/953 http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/764 http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1125

Comment on attachment 101269 [details] Patch v3 Attachment 101269 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9149039

(In reply to comment #13) > (From update of attachment 101269 [details]) > Attachment 101269 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/9149039 Umm, Qt still doesn't compile...

As discussed off-line, we've decided to go with the first version with a slight modification ("ui64" for #if COMPILER(MSVC), "ull" otherwise).

Created attachment 101281 [details] Patch v4

Comment on attachment 101281 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=101281&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:466 > +#if COMPILER(MSVC) > + static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFui64; > +#else > + static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFull; > +#endif nit: It's nice to have a comment that we wanted to use UINT64_C. > Source/WebCore/websockets/WebSocketChannel.cpp:468 > + if (payloadLength64 > maxPayloadLength || payloadLength64 + maskingKeyLength > std::numeric_limits<size_t>::max()) { nit: We usually declare "using namespace std;" at the beginning of the file and omit std::.

(In reply to comment #17) > nit: It's nice to have a comment that we wanted to use UINT64_C. I meant a FIXME comment.

Created attachment 101284 [details] Patch to land

Comment on attachment 101281 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=101281&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:466 >> +#endif > > nit: It's nice to have a comment that we wanted to use UINT64_C. Added. >> Source/WebCore/websockets/WebSocketChannel.cpp:468 >> + if (payloadLength64 > maxPayloadLength || payloadLength64 + maskingKeyLength > std::numeric_limits<size_t>::max()) { > > nit: We usually declare "using namespace std;" at the beginning of the file and omit std::. Fixed. Removed a couple of occurrences of "std::" in this file.

Comment on attachment 101284 [details] Patch to land Clearing flags on attachment: 101284 Committed r91243: <http://trac.webkit.org/changeset/91243>

All reviewed patches have been landed. Closing bug.