Bug 129193

Summary: CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify contains seemingly accidental unreachable code
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore Misc.Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, joepeck, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed fix none

Description Joseph Pecoraro 2014-02-21 20:39:19 PST
Compiling CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp with -Wunreachable-code catches:
Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:94:24: error: will never be executed [-Werror,-Wunreachable-code]

It looks like a legit warning:

    void CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify(const CryptoAlgorithmRsaSsaParams& parameters, const CryptoKeyRSA& key, const CryptoOperationData& signature, const CryptoOperationData& data, BoolCallback callback, VoidCallback failureCallback, ExceptionCode& ec)
    {
        ...

        CCCryptorStatus status = CCRSACryptorVerify(key.platformKey(), ccPKCS1Padding, digestData.data(), digestData.size(), digestAlgorithm, 0, signature.first, signature.second);
        if (!status)
            callback(true);
        else if (status == kCCNotVerified || kCCDecodeError) // <rdar://problem/15464982> CCRSACryptorVerify returns kCCDecodeError instead of kCCNotVerified sometimes
            callback(false);
        else
            failureCallback();
    }

---

The "|| kCCDecodeError" part looks suspicious. Perhaps that should be comparing with status, otherwise it looks like it will always be true, and the "else failureCallback()" will never be reached.
Comment 1 Alexey Proskuryakov 2014-02-21 20:51:01 PST
Created attachment 224943 [details]
proposed fix
Comment 2 Joseph Pecoraro 2014-02-21 21:20:03 PST
Comment on attachment 224943 [details]
proposed fix

r=me!

Should we write a test?
Comment 3 Alexey Proskuryakov 2014-02-21 21:30:14 PST
Comment on attachment 224943 [details]
proposed fix

> Should we write a test?

I don't think that there is any way to get to the failureCallback() branch in practice.
Comment 4 WebKit Commit Bot 2014-02-21 22:02:26 PST
Comment on attachment 224943 [details]
proposed fix

Clearing flags on attachment: 224943

Committed r164528: <http://trac.webkit.org/changeset/164528>
Comment 5 WebKit Commit Bot 2014-02-21 22:02:33 PST
All reviewed patches have been landed.  Closing bug.