[webkit-reviews] review denied: [Bug 125412] Type punning error in MD5.cpp : [Attachment 218692] Use memset

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brendan Long <b.long at cablelabs.com>'s
request for review:
Bug 125412: Type punning error in MD5.cpp
https://bugs.webkit.org/show_bug.cgi?id=125412

Attachment 218692: Use memset
https://bugs.webkit.org/attachment.cgi?id=218692&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218692&action=review


> Source/WTF/wtf/MD5.cpp:66
>      do {

Not new to this patch, but it’s a little nutty that we run all this code in
this reverseBytes function with the intention of doing nothing on little-endian
machines! Sure would be nice to not do that. And the name of the function is
silly since it does not reverse bytes on little-endian machines.

> Source/WTF/wtf/MD5.cpp:70
> -	   *reinterpret_cast_ptr<uint32_t *>(buf) = t;
> -	   buf += 4;
> +	   memset(buf, t, sizeof(t));
> +	   buf += sizeof(t);

The memset function converts its argument to an unsigned char, so this will
discard the high bits of "t" and give us incorrect results.

I’d expect this would be immediately visible because all MD5 digests would be
wrong all the time. Did you test the code after changing it?

> Source/WTF/wtf/MD5.cpp:252
>      // Append length in bits and transform
>      // m_in is 4-byte aligned.
> -    (reinterpret_cast_ptr<uint32_t*>(m_in))[14] = m_bits[0];
> -    (reinterpret_cast_ptr<uint32_t*>(m_in))[15] = m_bits[1];
> +    memcpy(m_in + 56, m_bits, sizeof(m_bits));

This change looks OK, but I would remove the comment “m_in is 4-byte aligned”
since the code no longer would be relying on that.