[webkit-reviews] review denied: [Bug 37913] MD5 is required for WebSocket new protocol implementation : [Attachment 53939] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Fumitoshi Ukai <ukai at chromium.org>'s
request for review:
Bug 37913: MD5 is required for WebSocket new protocol implementation
https://bugs.webkit.org/show_bug.cgi?id=37913

Attachment 53939: Patch
https://bugs.webkit.org/attachment.cgi?id=53939&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>	JavaScriptCore/wtf/Locker.h \
> +	   JavaScriptCore/wtf/MD5.cpp \
> +	   JavaScriptCore/wtf/MD5.h \
>	JavaScriptCore/wtf/MainThread.cpp \

Looks like existing entries here use tabs and your new entry uses spaces.

> +static void byteReverse(uint8_t *buf, unsigned longs)

The "*" should be next to the type, uint8_t.

The name here is not great. The words "byte reverse" are not a verb phrase. I
think reverseBytes would be better.

> +{
> +    uint32_t t;
> +    do {
> +	   t = static_cast<uint32_t>(static_cast<unsigned>(buf[3]) << 8 |
buf[2]) << 16 | (static_cast<unsigned>(buf[1]) << 8 | buf[0]);

The casts to unsigned here are not helpful. The code will work fine without
them, because the operation sizes are based on the type "unsigned" not the type
"uint8_t" due how the C language works.

> +	   *(reinterpret_cast<uint32_t *>(buf)) = t;

There is an extra set of parentheses here that is to needed.

This won't work on all WebKit platforms -- some CPUs require alignment. See
StringHash.h for an example of this.

> +	   buf += 4;
> +    } while (--longs);

Since this will malfunction if passed 0 for "longs" there should be an
assertion that longs is non-zero.

> +}
> +// The four core functions - F1 is optimized somewhat
> +

This comment is about the code below, so it should have a blank line before it
and possibly no blank line below it.

> +// #define F1(x, y, z) (x & y | ~x & z)
> +#define F1(x, y, z) (z ^ (x & (y ^ z)))

It is unclear to have the commented out F1 as well as the real F1. The comment
above saying "F1 is optimized somewhat" does not make things clear enough. I
think there's a better way to write the comment and say explicitly what's going
on.

> +#define F2(x, y, z) F1(z, x, y)
> +#define F3(x, y, z) (x ^ y ^ z)
> +#define F4(x, y, z) (y ^ (x | ~z))
> +
> +// This is the central step in the MD5 algorithm.
> +#define MD5STEP(f, w, x, y, z, data, s) \
> +    (w += f(x, y, z) + data,  w = w << s | w >> (32 - s),  w += x)

Extra spaces here after the commas.

> +    register uint32_t a, b, c, d;
> +
> +    a = buf[0];
> +    b = buf[1];
> +    c = buf[2];
> +    d = buf[3];

The use of "register" here is quaint and outdated. No compiler we use responds
to that keyword.

The variables should be declared when they are initialized, not on a separate
line.

> +    uint32_t t;
> +
> +    // Update bitcount
> +
> +    t = m_bits[0];

THe variable t should be initialized on the same line it's declared on.

> +    if ((m_bits[0] = t + (static_cast<uint32_t>(length) << 3)) < t)

Is the static_cast here needed?

Can this code be written with the assignment outside the if statement instead?

> +	   m_bits[1]++; // Carry from low to high
> +    m_bits[1] += static_cast<uint32_t>(length >> 29);

Is the static_cast here needed? Does it silence a warning?

> +	   MD5Transform(m_buf, reinterpret_cast<uint32_t *>(m_in));

The "*" goes next to the type.

This won't work on all WebKit platforms -- some CPUs require alignment. See
StringHash.h for an example of this.

> +	   MD5Transform(m_buf, reinterpret_cast<uint32_t *>(m_in));

The "*" goes next to the type.

This won't work on all WebKit platforms -- some CPUs require alignment. See
StringHash.h for an example of this.

> +Vector<uint8_t, 16> MD5::checksum()
> +{
> +    unsigned count;
> +    uint8_t* p;
> +
> +    // Compute number of bytes mod 64
> +    count = (m_bits[0] >> 3) & 0x3F;
> +
> +    // Set the first char of padding to 0x80.  This is safe since there is
> +    // always at least one byte free
> +    p = m_in + count;

THe variables count and p should be initialized on the same line they are
declared on.

> +	   // Two lots of padding:  Pad the first block to 64 bytes

I don't understand the term "lots of padding". What is a "lot"?

> +	   MD5Transform(m_buf, reinterpret_cast<uint32_t *>(m_in));

The "*" goes next to the type.

This won't work on all WebKit platforms -- some CPUs require alignment. See
StringHash.h for an example of this.

> +    // Append length in bits and transform
> +    (reinterpret_cast<uint32_t *>(m_in))[14] = m_bits[0];
> +    (reinterpret_cast<uint32_t *>(m_in))[15] = m_bits[1];
> +
> +    MD5Transform(m_buf, reinterpret_cast<uint32_t *>(m_in));

The "*" goes next to the type.

This won't work on all WebKit platforms -- some CPUs require alignment. See
StringHash.h for an example of this.

> +    byteReverse(reinterpret_cast<uint8_t *>(m_buf), 4);

The "*" goes next to the type.

Does this work on both little-endian and big-endian platforms?

> +#include <wtf/OwnPtr.h>
> +#include <wtf/Vector.h>

There is no reason to include the OwnPtr.h header here.

> +    ~MD5() { }

Please omit this destructor. It has no effect and matches what the compiler
would generate if you didn't write anything.

> +    void addBytes(const Vector<uint8_t>& input)
> +    {
> +	   addBytes(input.data(), input.size());
> +    }
> +    void addBytes(const uint8_t* input, size_t length);
> +    Vector<uint8_t, 16> checksum();

I think you need some sort of comment to make clear that calling checksum has a
side effect of resetting the state of the object. And the checksum function
should go in a separate paragraph.

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.

review- primarily because of the alignment issues