[webkit-reviews] review granted: [Bug 170550] [GCrypt] Implement AES_CBC support : [Attachment 306392] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 170550: [GCrypt] Implement AES_CBC support
https://bugs.webkit.org/show_bug.cgi?id=170550

Attachment 306392: Patch

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




--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 306392
  --> https://bugs.webkit.org/attachment.cgi?id=306392
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:69
> +	   size_t paddedSize =
roundUpToMultipleOf(gcry_cipher_get_algo_blklen(*algorithm), size + 1);

What's the +1 for?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:71
> +	   size_t padding = paddedSize - size;

I think paddingValue would be a clearer name for this variable.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:72
> +	   ASSERT(padding > 0);

I'm confused by this assert. Does padding really have to be greater than zero?
What happens if the message length is evenly divisible by block size? And,
although unlikely, what if the size of the message is one less than the size of
the block? You're sure this assert can never be triggered by web content?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:75
> +	   for (size_t i = size; i < paddedSize; ++i)
> +	       plainText[i] = padding;

You prefer writing out this loop to just using memcpy()? I wonder if memcpy()
might be more efficient.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmAES_CBCGCrypt.cpp:157
> +	   [parameters = WTFMove(parameters), key = WTFMove(key), plainText =
WTFMove(plainText), callback = WTFMove(callback), exceptionCallback =
WTFMove(exceptionCallback), &context]() mutable {

I should maybe have asked this before, but why does it have to be mutable?