Bug 129193 - CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify contains seemingly accidental unreachable code
Summary: CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify contains seemingly accidenta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-21 20:39 PST by Joseph Pecoraro
Modified: 2014-02-21 22:02 PST (History)
4 users (show)

See Also:


Attachments
proposed fix (1.69 KB, patch)
2014-02-21 20:51 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.