Bug 123582 – [WebCrypto] Add SHA-1

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

Implement SHA-1 glue code, and use Mac system libraries for actual implementation.

[Alexey Proskuryakov]

Created attachment 215662 [details]
proposed patch

[WebKit Commit Bot]

Attachment 215662 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/TestExpectations', u'LayoutTests/security/crypto-subtle-arguments-expected.txt', u'LayoutTests/security/crypto-subtle-arguments.html', u'LayoutTests/security/crypto-subtle-sha1-expected.txt', u'LayoutTests/security/crypto-subtle-sha1.html', u'LayoutTests/security/resources/common.js', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp', u'Source/WebCore/crypto/SubtleCrypto.idl', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h', u'Source/WebCore/crypto/mac/CryptoAlgorithmRegistryMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmSHA1Mac.cpp']" exit_code: 1
LayoutTests/TestExpectations:79:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Created attachment 215664 [details]
with a better skip list

[Anders Carlsson]

Comment on attachment 215664 [details]
with a better skip list

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

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:49
> +        return 0;

nullptr.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:55
> +    return std::move(result);

No need for std::move here.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h:38
> +    static const char* const m_name;
> +    static const CryptoAlgorithmIdentifier m_identifier = CryptoAlgorithmIdentifier::SHA_1;

We usually don’t use m_ for public member variables, at least I don’t think we do.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r158387>.

[Darin Adler]

Comment on attachment 215664 [details]
with a better skip list

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

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:52
> +    std::unique_ptr<CryptoAlgorithm> result(CryptoAlgorithmRegistry::shared().create(algorithmIdentifier));

I would write:

    auto result = CryptoAlgorithmRegistry::shared().create(algorithmIdentifier);

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:86
> +    std::unique_ptr<CryptoAlgorithm> algorithm = createAlgorithmFromJSValue(exec, exec->uncheckedArgument(0));

I like auto better than std::unique_ptr here.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:92
> +    std::unique_ptr<CryptoAlgorithmParameters> parameters = JSCryptoAlgorithmDictionary::createParametersForDigest(exec, algorithm->identifier(), exec->uncheckedArgument(0));

I like auto better than std::unique_ptr here.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmSHA1.h:38
>> +    static const CryptoAlgorithmIdentifier m_identifier = CryptoAlgorithmIdentifier::SHA_1;
> 
> We usually don’t use m_ for public member variables, at least I don’t think we do.

We definitely don’t use m_ for static data members. We normally avoid public member variables entirely, except when we use a struct instead of a class and we don’t use prefixes at all. I think it would be best to use a public static function instead of a public static data member. Inline if it needs to be.

> Source/WebCore/crypto/mac/CryptoAlgorithmSHA1Mac.cpp:36
> +void CryptoAlgorithmSHA1::digest(const CryptoAlgorithmParameters&, const Vector<CryptoOperationData>& data, std::unique_ptr<PromiseWrapper> promise, ExceptionCode&)

Seems like this should take PromiseWrapper& instead of unique_ptr<PromiseWrapper>.

[Alexey Proskuryakov]

> I like auto better than std::unique_ptr here.

Can try how this works. I'm worried that with smart pointers replaced with auto, it will be harder to confirm that there are no memory leaks.

> Seems like this should take PromiseWrapper& instead of unique_ptr<PromiseWrapper>.

The interface is designed to allow for async execution, so I don't think that this would work.