RESOLVED FIXED 125412
Type punning error in MD5.cpp
https://bugs.webkit.org/show_bug.cgi?id=125412
Summary Type punning error in MD5.cpp
Brendan Long
Reported 2013-12-08 06:24:59 PST
Type punning error in MD5.cpp
Attachments
Patch (1.37 KB, patch)
2013-12-08 06:28 PST, Brendan Long
no flags
Use memset (1.78 KB, patch)
2013-12-08 09:16 PST, Brendan Long
no flags
Patch (3.40 KB, patch)
2013-12-10 11:56 PST, Brendan Long
no flags
Brendan Long
Comment 1 2013-12-08 06:27:11 PST
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
Comment 2 2013-12-08 06:28:58 PST
Brendan Long
Comment 3 2013-12-08 08:56:05 PST
It was suggested that I use memcpy instead, since it's more efficient and possibly more clear.
Brendan Long
Comment 4 2013-12-08 09:16:15 PST
Created attachment 218692 [details] Use memset
Brendan Long
Comment 5 2013-12-08 09:17:29 PST
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 6 2013-12-09 11:07:10 PST
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
Comment 7 2013-12-09 11:08:17 PST
(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
Comment 8 2013-12-10 04:07:06 PST
(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
Comment 9 2013-12-10 11:50:22 PST
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
Comment 10 2013-12-10 11:56:06 PST
Brendan Long
Comment 11 2013-12-10 11:56:58 PST
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 12 2013-12-11 14:22:22 PST
Comment on attachment 218889 [details] Patch Looks fine.
WebKit Commit Bot
Comment 13 2013-12-11 14:57:04 PST
Comment on attachment 218889 [details] Patch Clearing flags on attachment: 218889 Committed r160460: <http://trac.webkit.org/changeset/160460>
WebKit Commit Bot
Comment 14 2013-12-11 14:57:06 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.