Bug 55039 – Add SHA-1 for new WebSocket protocol

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yuta Kitamura]

SHA-1 is required to implement the new WebSocket protocol draft discussed in IETF hybi working group (hybi-05 is the latest draft).
http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-05

[Yuta Kitamura]

Created attachment 83473 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 83473 [details]
Patch v1

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

WebSockets certainly looks like an severely overengineered protocol now.

Does this need to be in WTF, not in WebCore/platform?

> Source/JavaScriptCore/wtf/SHA1.cpp:47
> +static inline void testSHA1() { }

I don't think that it's appropriate to put test functions here.

> Source/JavaScriptCore/wtf/SHA1.cpp:81
> +static inline uint32_t f(int t, uint32_t b, uint32_t c, uint32_t d)

Ugh.

> Source/JavaScriptCore/wtf/SHA1.cpp:111
> +SHA1::SHA1()

This is not a good function name.

> Source/JavaScriptCore/wtf/SHA1.cpp:187
> +        uint32_t temp = s(5, a) + f(t, b, c, d) + e + k(t) + w[t];

Ugh. Do these variable names just come from the spec?

> Source/JavaScriptCore/wtf/SHA1.cpp:218
> +} // namesapce WTF

Typo: namesapse. I suggest removing these comments from whole file namespace blocks - they don't add anything except for visual noise. The only time they can be interesting is when you won't trust them anyway, and will use an editor feature to check matching braces.

> Source/JavaScriptCore/wtf/SHA1.h:31
> +// Based on Chromium's portable SHA-1 implementation (src/base/sha1_portable.cc).

This is a good comment to make in ChangeLog, not in source code.

> Source/JavaScriptCore/wtf/SHA1.h:51
> +    void checksum(Vector<uint8_t, 20>&);

Function names should be verbs.

[Yuta Kitamura]

Comment on attachment 83473 [details]
Patch v1

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

A bit more context of this patch: WTF::MD5 was introduced based on the same motivation in the past. I looked at structure and method interfaces of WTF::MD5 (testSHA1 function, constructor, addBytes, checksum) and tried to imitate them.

I'm open to moving this under WebCore/platform, but I feel that it's good to have it in WTF:

* When MD5 was introduced, WebSocketHandshake was the only user. Now Qt uses this library in WebKit/qt/WebCoreSupport/InspectorServerQt.cpp. I expect SHA-1 will also be used in other places like this example.
* Maciej once suggested putting MD5 into WTF (https://lists.webkit.org/pipermail/webkit-dev/2010-April/012576.html). I guess we can do the same...

>> Source/JavaScriptCore/wtf/SHA1.cpp:47
>> +static inline void testSHA1() { }
> 
> I don't think that it's appropriate to put test functions here.

Darin Adler requested unit-testing MD5 class when he reviewed it:
https://bugs.webkit.org/show_bug.cgi?id=37913#c3
| I know that often we have classes with no testing. But it's critical we have a unit test for this class somewhere.
| If nothing else, people porting will need to know that this compiled and works properly. It could be as simple as
| having a debug only function that runs the unit test in debug builds the first time the MD5 constructor is called.

Do you have any idea about where to put tests?

>> Source/JavaScriptCore/wtf/SHA1.cpp:81
>> +static inline uint32_t f(int t, uint32_t b, uint32_t c, uint32_t d)
> 
> Ugh.

Unfortunately this name is used in RFC3174 (section 5). It's also hard to give a good name to this function...

>> Source/JavaScriptCore/wtf/SHA1.cpp:111
>> +SHA1::SHA1()
> 
> This is not a good function name.

I don't think of good alternatives; maybe SHA1HashCalculator?

>> Source/JavaScriptCore/wtf/SHA1.cpp:187
>> +        uint32_t temp = s(5, a) + f(t, b, c, d) + e + k(t) + w[t];
> 
> Ugh. Do these variable names just come from the spec?

Sadly, yes. In RFC 3174 section 6.1:

  d. For t = 0 to 79 do
     TEMP = S^5(A) + f(t;B,C,D) + E + W(t) + K(t);
     E = D;  D = C;  C = S^30(B);  B = A; A = TEMP;

>> Source/JavaScriptCore/wtf/SHA1.cpp:218
>> +} // namesapce WTF
> 
> Typo: namesapse. I suggest removing these comments from whole file namespace blocks - they don't add anything except for visual noise. The only time they can be interesting is when you won't trust them anyway, and will use an editor feature to check matching braces.

I sometimes find these comments helpful especally when the block is so long that it doesn't fit in the screen.

There was the same discussion in webkit-dev in the past (thread starting from https://lists.webkit.org/pipermail/webkit-dev/2010-August/013759.html). Apparently there was no consensus reached, though.

Anyway, I will fix the typo at least.

>> Source/JavaScriptCore/wtf/SHA1.h:31
>> +// Based on Chromium's portable SHA-1 implementation (src/base/sha1_portable.cc).
> 
> This is a good comment to make in ChangeLog, not in source code.

Will move.

>> Source/JavaScriptCore/wtf/SHA1.h:51
>> +    void checksum(Vector<uint8_t, 20>&);
> 
> Function names should be verbs.

Will fix.

[Shinichiro Hamaji]

Comment on attachment 83473 [details]
Patch v1

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

I don't know about websocket protocol. Sorry if my comments make few sense...

> Source/JavaScriptCore/wtf/SHA1.cpp:105
> +static inline uint32_t s(int n, uint32_t x)

We can call this as rotateLeft or something like this?

> Source/JavaScriptCore/wtf/SHA1.cpp:186
> +    for (int t = 0; t < 80; ++t) {

I guess most implementations split this loop into 4 for-loop for performance. I don't know if performance is important here though.

> Source/JavaScriptCore/wtf/SHA1.h:60
> +    size_t m_cursor; // Number of bytes filled in m_buffer (0-64).
> +    size_t m_totalBytes; // Number of bytes added so far.

Won't we need to calculate sha1 of >4GB stream?

[Yuta Kitamura]

Comment on attachment 83473 [details]
Patch v1

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

>> Source/JavaScriptCore/wtf/SHA1.cpp:105
>> +static inline uint32_t s(int n, uint32_t x)
> 
> We can call this as rotateLeft or something like this?

Yes, I will rename.

>> Source/JavaScriptCore/wtf/SHA1.cpp:186
>> +    for (int t = 0; t < 80; ++t) {
> 
> I guess most implementations split this loop into 4 for-loop for performance. I don't know if performance is important here though.

Performance is not an important factor at least for WebSocket protocol implementation (a short message will be hashed for each WebSocket handshake). I'd like to keep the code simple for now... I can certainly tune up the implementation when a performance problem comes up.

>> Source/JavaScriptCore/wtf/SHA1.h:60
>> +    size_t m_totalBytes; // Number of bytes added so far.
> 
> Won't we need to calculate sha1 of >4GB stream?

Will change this line to "uint64_t m_totalBytes".

[Yuta Kitamura]

Created attachment 83634 [details]
Patch v2

[Yuta Kitamura]

>> Source/JavaScriptCore/wtf/SHA1.cpp:111
>> +SHA1::SHA1()
> 
> This is not a good function name.

I'm still wondering about this. SHA1Calculator? SHA1Builder? I think just "SHA1" makes the most sense, too...

Any suggestions will be appreciated.

[Yuta Kitamura]

Could anybody review this patch? Or, could you suggest an appropriate reviewer?

Thanks in advance.

[Kent Tamura]

Comment on attachment 83634 [details]
Patch v2

I don't like having tests in SHA1.cpp and they are called by the constructor though I understand it's same as MD5.
Would you file a bug about the test location of SHA1 and MD5, and add a FIXME comment please?

[Yuta Kitamura]

(In reply to comment #9)
> (From update of attachment 83634 [details])
> I don't like having tests in SHA1.cpp and they are called by the constructor though I understand it's same as MD5.
> Would you file a bug about the test location of SHA1 and MD5, and add a FIXME comment please?

Filed bug 55853. I will add FIXMEs and commit manually. Thanks!

[Yuta Kitamura]

Committed r80446: <http://trac.webkit.org/changeset/80446>