[webkit-reviews] review denied: [Bug 207238] [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength longer than 64 : [Attachment 390498] Patch

[bugzilla-daemon at webkit.org]

Jiewen Tan <jiewen_tan at apple.com> has denied Yoshiaki Jitsukawa
<yoshiaki.jitsukawa at sony.com>'s request for review:
Bug 207238: [WebCrypto][CommonCrypto] Incorrect AES-CTR with counterLength
longer than 64
https://bugs.webkit.org/show_bug.cgi?id=207238

Attachment 390498: Patch

https://bugs.webkit.org/attachment.cgi?id=390498&action=review




--- Comment #14 from Jiewen Tan <jiewen_tan at apple.com> ---
Comment on attachment 390498
  --> https://bugs.webkit.org/attachment.cgi?id=390498
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390498&action=review

Looks good overall. Please address my comments.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:45
> +	   static constexpr size_t BlockSize = 16;

Could this be in the implementation?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:47
> +	   explicit CounterBlockHelper(const Vector<uint8_t>& counterVector,
size_t counterLength);

Since the constructor takes two parameters, there are no needs to make it
explicit.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:50
> +	   size_t countToOverflow() const;

In WebKit, we usually don't describe what the method does in comments. Instead,
we make the method name be self explanatory.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:53
> +	   Vector<uint8_t> counterVectorAfterOverflow() const;

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:56
> +	   struct CounterBlockBits {

If this is an internal helper, it should be private.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:57
> +	       static constexpr uint64_t All = 0xFFFFFFFFFFFFFFFFUL;

Could this be in the implementation?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:58
> +	       void set()

We usually don't put methods into the headers unless they are simple getters or
for performance reasons.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:64
> +	       bool all() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:69
> +	       bool any() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:74
> +	       CounterBlockBits operator&(const CounterBlockBits& rhs) const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:82
> +	       CounterBlockBits operator~() const

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:90
> +	       CounterBlockBits& operator <<=(unsigned shift)

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:106
> +	       CounterBlockBits& operator &=(const CounterBlockBits& rhs)

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CTR.h:108
> +		   *this = *this & rhs;

I believe this will create an unnecessary temp value. You should just modify
the members.