[webkit-reviews] review denied: [Bug 170270] [GCrypt] Implement PBKDF2 support : [Attachment 305947] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied  review:
Bug 170270: [GCrypt] Implement PBKDF2 support
https://bugs.webkit.org/show_bug.cgi?id=170270

Attachment 305947: Patch

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




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

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

Actually, I'd like to look at this one again.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:64
> +    Vector<uint8_t> result(length / 8);

Can you explain why the length of the result is only 1/8th the size of the
length of the input? Seems a bit arbitrary?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:71
> +    return WTFMove(result);

Hm, no this is not right, you can't use WTFMove when returning a local
variable... it's stack-allocated and has been destroyed, right...? Isn't this
undefined behavior or something otherwise bad? Evens if I'm wrong about that,
you're guaranteed move semantics if you just return result directly anyway, so
you should do that instead, no need for WTFMove here.