Bug 170270 – [GCrypt] Implement PBKDF2 support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GCrypt] Implement PBKDF2 support

[Zan Dobersek]

Created attachment 305837 [details]
Patch

[Zan Dobersek]

Depends on the Utilities.h header that's being added in bug #170269.

[Zan Dobersek]

Created attachment 305947 [details]
Patch

[Zan Dobersek]

*** Bug 169528 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

Comment on attachment 305947 [details]
Patch

Thanks for adding WebCrypto API support for GTK+.
Looks good:)

[Michael Catanzaro]

Comment on attachment 305947 [details]
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:37
> +#include <pal/crypto/gcrypt/Utilities.h>

Can you not #include <pal/Utilities.h>? Do we not have any forwarding headers for PAL yet?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:76
> +    context.ref();

It'd be cleaner to use Ref<ScriptExecutionContext> for this, so you don't have to remember to deref it in two difference places below, but now I see that ScriptExecutionContext does not inherit from RefCounted even though it has ref and deref methods. Huh. So not great, but I guess this is best....

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:85
> +                    [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) {

What, why are you capturing callback here, when you don't use it? There's no particular need to move the value. Just ignore it.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:93
> +                [output = WTFMove(*output), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) {

And here there is no need to capture exceptionCallback.

[Michael Catanzaro]

Comment on attachment 305947 [details]
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.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7) 
> 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?

I guess in a function named "deriveBits" it's understandable that the length parameter is assumed to refer to bits, then you store it in a vector holding bytes... so OK, that checks out.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> 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.

This stupid complicated Stack Overflow answer says (a) I'm wrong and (b) you still shouldn't do it:

http://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value

So don't do it!

[Michael Catanzaro]

Comment on attachment 305947 [details]
Patch

(Changed my mind again; these are minor comments. ;)

[Zan Dobersek]

Comment on attachment 305947 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:37
>> +#include <pal/crypto/gcrypt/Utilities.h>
> 
> Can you not #include <pal/Utilities.h>? Do we not have any forwarding headers for PAL yet?

No, there's no forwarding headers for PAL.

>> 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?

The length is the number of bits to be derived, but the API works with bytes. CryptoAlgorithmPBKDF2::deriveBits() ensures that the length is a multiple of 8. An assert with a comment can be put here though.

>> 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.

This constructs the std::optional<Vector<uint8_t>> by moving the non-nullopt value into the std::optional<> constructor, which will then just move the resources of the stack-allocated Vector into the Vector that's wrapped in std::optional.

Move semantics would be guaranteed if the return type of this function was Vector<uint8_t>, but since it's std::optional<>, we have to construct that object upon return.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:85
>> +                    [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) {
> 
> What, why are you capturing callback here, when you don't use it? There's no particular need to move the value. Just ignore it.

This follows the Mac implementation. Per comments there, the callbacks should be dereferenced only when finally back on the Document or Worker thread.I'll add the same comment here, and in the other patches.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #11)
> Comment on attachment 305947 [details]
> >> 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.
> 
> This constructs the std::optional<Vector<uint8_t>> by moving the non-nullopt
> value into the std::optional<> constructor, which will then just move the
> resources of the stack-allocated Vector into the Vector that's wrapped in
> std::optional.
> 
> Move semantics would be guaranteed if the return type of this function was
> Vector<uint8_t>, but since it's std::optional<>, we have to construct that
> object upon return.
> 

Testing this on a function that returns std::optional<std::unique_ptr<T>>, a simple return appears to work, constructing the std::optional<> object with an rvalue reference of the returned std::unique_ptr<> object without using move semantics.

I'll check at some point later whether the result Vector<> object is still copied when constructing the std::optional<> value, but even copies are probably not that expensive in these cases as the vectors are small.

So WTFMove() can be dropped.

[Zan Dobersek]

Created attachment 306064 [details]
Patch for landing

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #11) 
> This follows the Mac implementation. Per comments there, the callbacks
> should be dereferenced only when finally back on the Document or Worker
> thread.I'll add the same comment here, and in the other patches.

Wow, that's a serious footgun. Definitely requires a comment everywhere.

[Zan Dobersek]

Comment on attachment 306064 [details]
Patch for landing

Clearing flags on attachment: 306064

Committed r214821: <http://trac.webkit.org/changeset/214821>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.