(1 << counterLength) causes an integer overflow, and the undefined behavior. The longest valid counterLength on 64 bit machine is 63, and the literal 1 is considered as 32-bit signed integer. Left shifting 1 beyond or to sign-bit is undefined behavior in C++ spec. This issue is originally found in https://bugs.webkit.org/show_bug.cgi?id=208186#c2 Unfortunately, I don't own Mac so I can't test this locally. For OpenSSL implementation the added test case could catch this issue.
Created attachment 396514 [details] patch
Comment on attachment 396514 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=396514&action=review Good catch! Please address my comments below. > Source/WebCore/ChangeLog:6 > + (1 << counterLength) causes an integer overflow, and the undefined behavior. Maybe you could reference here: https://en.cppreference.com/w/cpp/language/integer_literal. > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) ((size_t)1 => 1ull
<rdar://problem/61857969>
(In reply to Jiewen Tan from comment #2) > Comment on attachment 396514 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > Good catch! Please address my comments below. Thanks for your review! > > > Source/WebCore/ChangeLog:6 > > + (1 << counterLength) causes an integer overflow, and the undefined behavior. > > Maybe you could reference here: > https://en.cppreference.com/w/cpp/language/integer_literal. Will do. > > > Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > > + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > > ((size_t)1 => 1ull I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. How do you think about it? Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull.
Comment on attachment 396514 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=396514&action=review >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) >> >> ((size_t)1 => 1ull > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > How do you think about it? > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. I was not aware we are still supporting 32 bit machines. Doing a C style cast is not appropriate in WebKit, please change it to static_cast<size_t>(1).
Created attachment 396737 [details] patch Reflected the review comments
(In reply to Jiewen Tan from comment #5) > Comment on attachment 396514 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > >> > >> ((size_t)1 => 1ull > > > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > > > How do you think about it? > > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. > > I was not aware we are still supporting 32 bit machines. Doing a C style > cast is not appropriate in WebKit, please change it to > static_cast<size_t>(1). I changed it to static_cast<size_t>(1). I don't know about Mac implementation, but the other code seems to try to support 32-bit by using size_t. > > > Source/WebCore/ChangeLog:6 > > + (1 << counterLength) causes an integer overflow, and the undefined behavior. > > Maybe you could reference here: > https://en.cppreference.com/w/cpp/language/integer_literal. I added - https://en.cppreference.com/w/cpp/language/integer_literal and - https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators as references.
(In reply to Tomoki Imai from comment #7) > (In reply to Jiewen Tan from comment #5) > > Comment on attachment 396514 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396514&action=review > > > > >>> Source/WebCore/crypto/mac/CryptoAlgorithmAES_CTRMac.cpp:48 > > >>> + if (counterLength < sizeof(size_t) * 8 && numberOfBlocks > ((size_t)1 << counterLength)) > > >> > > >> ((size_t)1 => 1ull > > > > > > I used ((size_t) 1) rather than 1ull, because I thought using size_t type makes more sense than "unsigned long long". > > > - "counterLength < sizeof(size_t) * 8" checks whether size_t has enough size to store (1 << counterLength) > > > - In 32-bit environment, 1ull, which is 64-bit or larger, is a bit overkill, because counterLength < 31 in 32-bit environment. > > > > > > How do you think about it? > > > Of course, 1ull should work fine both on 32-bit and 64-bit environment, so I'm fine to change it to 1ull. > > > > I was not aware we are still supporting 32 bit machines. Doing a C style > > cast is not appropriate in WebKit, please change it to > > static_cast<size_t>(1). > > I changed it to static_cast<size_t>(1). > I don't know about Mac implementation, but the other code seems to try to > support 32-bit by using size_t. > That was true 4 years ago for Apple ports but not now. Anyway, I think it is better to keep the consistency.
Comment on attachment 396737 [details] patch r=me.
Committed r260238: <https://trac.webkit.org/changeset/260238> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396737 [details].