Bug 125412 – Type punning error in MD5.cpp

• Component: New Bugs
• Status: RESOLVED FIXED

[Brendan Long]

Type punning error in MD5.cpp

[Brendan Long]

  CXX      Source/WTF/wtf/libWTF_la-MD5.lo
../../Source/WTF/wtf/MD5.cpp: In member function `void WTF::MD5::checksum(WTF::Vector<unsigned char, 16ul>&)':
../../Source/WTF/wtf/MD5.cpp:252:40: error: invalid cast from type `uint8_t {aka unsigned char}' to type `uint32_t {aka unsigned int}'
     reinterpret_cast_ptr<uint32_t>(m_in[56]) = m_bits[0];
                                        ^
../../Source/WTF/wtf/MD5.cpp:253:40: error: invalid cast from type `uint8_t {aka unsigned char}' to type `uint32_t {aka unsigned int}'
     reinterpret_cast_ptr<uint32_t>(m_in[60]) = m_bits[1];
                                        ^
make: *** [Source/WTF/wtf/libWTF_la-MD5.lo] Error 1


I'm not exactly sure why this is bad, but from what I've read, this is a real problem because compilers are allowed to assume that this will never happen (for optimization purposes). We *are* allowed to cast pointers to char*, so I just reversed this to case the uint32_t to uint8_t instead of casting the uint8_t to uint32_t.

[Brendan Long]

Created attachment 218685 [details]
Patch

[Brendan Long]

It was suggested that I use memcpy instead, since it's more efficient and possibly more clear.

[Brendan Long]

Created attachment 218692 [details]
Use memset

[Brendan Long]

By the way, this was originally copied from SQLite, but it doesn't exist in their codebase anymore (otherwise I would fix it there too).

[Darin Adler]

Comment on attachment 218692 [details]
Use memset

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.

[Darin Adler]

(In reply to comment #5)
> By the way, this was originally copied from SQLite, but it doesn't exist in their codebase anymore (otherwise I would fix it there too).

Does SQLite have a newer version of the MD5 algorithm. Maybe we should adopt that?

[Brendan Long]

(In reply to comment #7)
> Does SQLite have a newer version of the MD5 algorithm. Maybe we should adopt that?

No, they just removed it. Apparently md5.c was only used for tests of some kind.

[Brendan Long]

I thought I ran WebKitBuild/Debug/Programs/TestWebKitAPI/TestWTF --gtest_filter=WTF_MD5* before, but apparently I did something wrong, since it definitely fails.. I'll fix this with memcpy.

[Brendan Long]

Created attachment 218889 [details]
Patch

[Brendan Long]

I fixed things based on your comments, and the relevant test output is:

$ WebKitBuild/Debug/Programs/TestWebKitAPI/TestWTF --gtest_filter=WTF_MD5*
Note: Google Test filter = WTF_MD5*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WTF_MD5
[ RUN      ] WTF_MD5.Computation
[       OK ] WTF_MD5.Computation (0 ms)
[----------] 1 test from WTF_MD5 (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.

[Darin Adler]

Comment on attachment 218889 [details]
Patch

Looks fine.

[WebKit Commit Bot]

Comment on attachment 218889 [details]
Patch

Clearing flags on attachment: 218889

Committed r160460: <http://trac.webkit.org/changeset/160460>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.