128657 2014-02-11 23:50:08 -0800 Don't crash when SerializedScriptValue deserialization fails 2014-02-12 20:00:18 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ap ap alecflett commit-queue ggaren jsbell oliver oldest_to_newest 979785 0 ap 2014-02-11 23:50:08 -0800 There are multiple reasons for SerializedScriptValue deserialization to fail: - as it's stored to disk persistently, the storage could be corrupted; - CryptoKey deserialization only works on main thread, not in workers; - soon, CryptoKey deserialization will fail if we can't get a master key from Keychain. So, we need to handle this gracefully. 979791 1 223947 ap 2014-02-12 00:07:58 -0800 Created attachment 223947 proposed fix 979873 2 223947 oliver 2014-02-12 09:02:19 -0800 Comment on attachment 223947 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=223947&action=review r- due to the 0->jsNull change > Source/WebCore/bindings/js/JSMessageEventCustom.cpp:67 > + // FIXME: Why does this suppress exceptions? Check with stephanie - i can't remember whether the change responsible for this code was to make some api thingy work, or to refactor it to this layout was hers or mine > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2654 > - return 0; > + return toRef(exec, jsNull()); Our api for deserialising is to return NULL on failure, not a jsvalue -- nullptr is distinguishable from jsNull. This change makes it impossible to distinguish desrialisation failure, and deserialing jsNull > LayoutTests/crypto/subtle/postMessage-worker-expected.txt:6 > -PASS All checks passed in worker > -PASS key.type is 'secret' > -PASS key.extractable is true > -PASS key.algorithm.name is 'HMAC' > -PASS key.algorithm.length is 16 > -PASS key.usages is ["decrypt", "encrypt", "sign", "verify"] > +FAIL Check failed in worker: key is null wut? 979901 3 ap 2014-02-12 10:24:33 -0800 > Check with stephanie - i can't remember whether the change responsible for this code was to make some api thingy work, or to refactor it to this layout was hers or mine It was neither Stephanie nor you - this instance of NonThrowing was added by Chromium people without any explanation, see <http://trac.webkit.org/changeset/93766>. > Our api for deserialising is to return NULL on failure, not a jsvalue -- nullptr is distinguishable from jsNull. This change makes it impossible to distinguish desrialisation failure, and deserialing jsNull That's WebKit1 SPI only, one that appears 100% unused. WebKit2 reports an exception Do you think that we could change or remove the WebKit1 SPI instead? > > LayoutTests/crypto/subtle/postMessage-worker-expected.txt:6 > > -PASS All checks passed in worker > > -PASS key.type is 'secret' > > -PASS key.extractable is true > > -PASS key.algorithm.name is 'HMAC' > > -PASS key.algorithm.length is 16 > > -PASS key.usages is ["decrypt", "encrypt", "sign", "verify"] > > +FAIL Check failed in worker: key is null > > wut? This is expected - the test passed before I added a delegate, and then started to crash because the delegate is not implemented in workers. Now it just fails, which is a progression. Also, it doesn't matter, because we don't expose WebCrypto in workers, so CryptoKey is of no use there. 979910 4 oliver 2014-02-12 10:41:33 -0800 (In reply to comment #3) > > Check with stephanie - i can't remember whether the change responsible for this code was to make some api thingy work, or to refactor it to this layout was hers or mine > > It was neither Stephanie nor you - this instance of NonThrowing was added by Chromium people without any explanation, see <http://trac.webkit.org/changeset/93766>. > Kill it with fire. > > Our api for deserialising is to return NULL on failure, not a jsvalue -- nullptr is distinguishable from jsNull. This change makes it impossible to distinguish desrialisation failure, and deserialing jsNull > > That's WebKit1 SPI only, one that appears 100% unused. WebKit2 reports an exception Old safari's may use it? > > Do you think that we could change or remove the WebKit1 SPI instead? > > > > LayoutTests/crypto/subtle/postMessage-worker-expected.txt:6 > > > -PASS All checks passed in worker > > > -PASS key.type is 'secret' > > > -PASS key.extractable is true > > > -PASS key.algorithm.name is 'HMAC' > > > -PASS key.algorithm.length is 16 > > > -PASS key.usages is ["decrypt", "encrypt", "sign", "verify"] > > > +FAIL Check failed in worker: key is null > > > > wut? > > This is expected - the test passed before I added a delegate, and then started to crash because the delegate is not implemented in workers. Now it just fails, which is a progression. Also, it doesn't matter, because we don't expose WebCrypto in workers, so CryptoKey is of no use there. Righto 979911 5 223947 oliver 2014-02-12 10:41:52 -0800 Comment on attachment 223947 proposed fix r+ based on alexia's comments 979924 6 ap 2014-02-12 11:09:24 -0800 > > That's WebKit1 SPI only, one that appears 100% unused. WebKit2 reports an exception > > Old safari's may use it? Very old ones. I guess I'll post a separate patch to remove the SPI, and maybe one to remove NonThrowing in the discussed code path too. 979927 7 ap 2014-02-12 11:14:52 -0800 That said, WebKit2 SPI clients are not very good about checking the exception value, so I'll keep the API helper returning null as you initially suggested. 980149 8 ap 2014-02-12 20:00:18 -0800 Committed <http://trac.webkit.org/r164008>. 223947 2014-02-12 00:07:58 -0800 2014-02-12 10:41:52 -0800 proposed fix DeserializationCrash.txt text/plain 5787 ap SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2Mzk1MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE0LTAyLTExICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIERvbid0IGNyYXNoIHdoZW4g U2VyaWFsaXplZFNjcmlwdFZhbHVlIGRlc2VyaWFsaXphdGlvbiBmYWlscworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI4NjU3CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGVzdDogY3J5cHRvL3N1YnRsZS9w b3N0TWVzc2FnZS13b3JrZXIuaHRtbAorCisgICAgICAgICogYmluZGluZ3MvanMvSlNNZXNzYWdl RXZlbnRDdXN0b20uY3BwOiAoV2ViQ29yZTo6SlNNZXNzYWdlRXZlbnQ6OmRhdGEpOiBBZGRlZCBh IEZJWE1FLgorCisgICAgICAgICogYmluZGluZ3MvanMvU2VyaWFsaXplZFNjcmlwdFZhbHVlLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OkNsb25lQmFzZTo6ZmFpbCk6IERvbid0IGFzc2VydCBvbiBm YWlsdXJlLgorICAgICAgICAoV2ViQ29yZTo6U2VyaWFsaXplZFNjcmlwdFZhbHVlOjpkZXNlcmlh bGl6ZSk6IE5ldmVyIHJldHVybiBhIG51bGwgSlNWYWx1ZSwgdGhlc2UKKyAgICAgICAgYXJlIG5v dCBhbGxvd2VkLgorCiAyMDE0LTAyLTExICBaYWxhbiBCdWp0YXMgIDx6YWxhbkBhcHBsZS5jb20+ CiAKICAgICAgICAgU3VicGl4ZWwgcmVuZGVyaW5nOiBTd2l0Y2ggcmVwYWludCByZWN0IGZyb20g SW50UmVjdCB0byBMYXlvdXRSZWN0IHRvIGJlIGFibGUgdG8KSW5kZXg6IFNvdXJjZS9XZWJDb3Jl L2JpbmRpbmdzL2pzL0pTTWVzc2FnZUV2ZW50Q3VzdG9tLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU01lc3NhZ2VFdmVudEN1c3RvbS5jcHAJKHJldmlzaW9u IDE2Mzk0OCkKKysrIFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTTWVzc2FnZUV2ZW50Q3Vz dG9tLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjQsOSArNjQsOSBAQCBKU1ZhbHVlIEpTTWVzc2Fn ZUV2ZW50OjpkYXRhKEV4ZWNTdGF0ZSogCiAgICAgY2FzZSBNZXNzYWdlRXZlbnQ6OkRhdGFUeXBl U2VyaWFsaXplZFNjcmlwdFZhbHVlOgogICAgICAgICBpZiAoUmVmUHRyPFNlcmlhbGl6ZWRTY3Jp cHRWYWx1ZT4gc2VyaWFsaXplZFZhbHVlID0gZXZlbnQuZGF0YUFzU2VyaWFsaXplZFNjcmlwdFZh bHVlKCkpIHsKICAgICAgICAgICAgIE1lc3NhZ2VQb3J0QXJyYXkgcG9ydHMgPSBpbXBsKCkucG9y dHMoKTsKKyAgICAgICAgICAgIC8vIEZJWE1FOiBXaHkgZG9lcyB0aGlzIHN1cHByZXNzIGV4Y2Vw dGlvbnM/CiAgICAgICAgICAgICByZXN1bHQgPSBzZXJpYWxpemVkVmFsdWUtPmRlc2VyaWFsaXpl KGV4ZWMsIGdsb2JhbE9iamVjdCgpLCAmcG9ydHMsIE5vblRocm93aW5nKTsKLSAgICAgICAgfQot ICAgICAgICBlbHNlCisgICAgICAgIH0gZWxzZQogICAgICAgICAgICAgcmVzdWx0ID0ganNOdWxs KCk7CiAgICAgICAgIGJyZWFrOwogCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9T ZXJpYWxpemVkU2NyaXB0VmFsdWUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2Jp bmRpbmdzL2pzL1NlcmlhbGl6ZWRTY3JpcHRWYWx1ZS5jcHAJKHJldmlzaW9uIDE2Mzk0OCkKKysr IFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL1NlcmlhbGl6ZWRTY3JpcHRWYWx1ZS5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTM4MSwxMCArMzgxLDggQEAgcHJvdGVjdGVkOgogICAgICAgICBtX2V4 ZWMtPnZtKCkudGhyb3dFeGNlcHRpb24obV9leGVjLCBjcmVhdGVTdGFja092ZXJmbG93RXJyb3Io bV9leGVjKSk7CiAgICAgfQogCi0gICAgTk9fUkVUVVJOX0RVRV9UT19BU1NFUlQKICAgICB2b2lk IGZhaWwoKQogICAgIHsKLSAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAgICAgICAgIG1f ZmFpbGVkID0gdHJ1ZTsKICAgICB9CiAKQEAgLTI2NDEsNyArMjYzOSw3IEBAIEpTVmFsdWUgU2Vy aWFsaXplZFNjcmlwdFZhbHVlOjpkZXNlcmlhbGkKICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG1fYXJyYXlCdWZmZXJDb250 ZW50c0FycmF5LmdldCgpLCBtX2RhdGEpOwogICAgIGlmICh0aHJvd0V4Y2VwdGlvbnMgPT0gVGhy b3dpbmcpCiAgICAgICAgIG1heWJlVGhyb3dFeGNlcHRpb25JZlNlcmlhbGl6YXRpb25GYWlsZWQo ZXhlYywgcmVzdWx0LnNlY29uZCk7Ci0gICAgcmV0dXJuIHJlc3VsdC5maXJzdDsKKyAgICByZXR1 cm4gcmVzdWx0LmZpcnN0ID8gcmVzdWx0LmZpcnN0IDoganNOdWxsKCk7CiB9CiAKIEpTVmFsdWVS ZWYgU2VyaWFsaXplZFNjcmlwdFZhbHVlOjpkZXNlcmlhbGl6ZShKU0NvbnRleHRSZWYgZGVzdGlu YXRpb25Db250ZXh0LCBKU1ZhbHVlUmVmKiBleGNlcHRpb24pCkBAIC0yNjUzLDcgKzI2NTEsNyBA QCBKU1ZhbHVlUmVmIFNlcmlhbGl6ZWRTY3JpcHRWYWx1ZTo6ZGVzZXJpCiAgICAgICAgIGlmIChl eGNlcHRpb24pCiAgICAgICAgICAgICAqZXhjZXB0aW9uID0gdG9SZWYoZXhlYywgZXhlYy0+ZXhj ZXB0aW9uKCkpOwogICAgICAgICBleGVjLT5jbGVhckV4Y2VwdGlvbigpOwotICAgICAgICByZXR1 cm4gMDsKKyAgICAgICAgcmV0dXJuIHRvUmVmKGV4ZWMsIGpzTnVsbCgpKTsKICAgICB9CiAgICAg QVNTRVJUKHZhbHVlKTsKICAgICByZXR1cm4gdG9SZWYoZXhlYywgdmFsdWUpOwpJbmRleDogTGF5 b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwko cmV2aXNpb24gMTYzOTUyKQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp CkBAIC0xLDMgKzEsMTUgQEAKKzIwMTQtMDItMTIgIEFsZXhleSBQcm9za3VyeWFrb3YgIDxhcEBh cHBsZS5jb20+CisKKyAgICAgICAgRG9uJ3QgY3Jhc2ggd2hlbiBTZXJpYWxpemVkU2NyaXB0VmFs dWUgZGVzZXJpYWxpemF0aW9uIGZhaWxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMjg2NTcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGNyeXB0by9zdWJ0bGUvcG9zdE1lc3NhZ2Utd29ya2VyLWV4cGVj dGVkLnR4dDoKKyAgICAgICAgKiBjcnlwdG8vc3VidGxlL3Jlc291cmNlcy9wb3N0TWVzc2FnZS13 b3JrZXIuanM6CisgICAgICAgICogcGxhdGZvcm0vbWFjL1Rlc3RFeHBlY3RhdGlvbnM6CisgICAg ICAgIFVuc2tpcCB0aGUgdGVzdCwgYW5kIGxhbmQgKHVuaW1wb3J0YW50KSBmYWlsdXJlIHJlc3Vs dHMuCisKIDIwMTQtMDItMTEgIEFsZXhleSBQcm9za3VyeWFrb3YgIDxhcEBhcHBsZS5jb20+CiAK ICAgICAgICAgVW5kbyBhbiBhY2NpZGVudGFsIGNoYW5nZS4KSW5kZXg6IExheW91dFRlc3RzL2Ny eXB0by9zdWJ0bGUvcG9zdE1lc3NhZ2Utd29ya2VyLWV4cGVjdGVkLnR4dAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBMYXlvdXRUZXN0cy9jcnlwdG8vc3VidGxlL3Bvc3RNZXNzYWdlLXdvcmtlci1leHBlY3RlZC50 eHQJKHJldmlzaW9uIDE2Mzk0OCkKKysrIExheW91dFRlc3RzL2NyeXB0by9zdWJ0bGUvcG9zdE1l c3NhZ2Utd29ya2VyLWV4cGVjdGVkLnR4dAkod29ya2luZyBjb3B5KQpAQCAtMywxMiArMyw3IEBA IFRlc3Qgc2VuZGluZyBjcnlwdG8ga2V5cyB2aWEgcG9zdE1lc3NhZ2UKIE9uIHN1Y2Nlc3MsIHlv dSB3aWxsIHNlZSBhIHNlcmllcyBvZiAiUEFTUyIgbWVzc2FnZXMsIGZvbGxvd2VkIGJ5ICJURVNU IENPTVBMRVRFIi4KIAogCi1QQVNTIEFsbCBjaGVja3MgcGFzc2VkIGluIHdvcmtlcgotUEFTUyBr ZXkudHlwZSBpcyAnc2VjcmV0JwotUEFTUyBrZXkuZXh0cmFjdGFibGUgaXMgdHJ1ZQotUEFTUyBr ZXkuYWxnb3JpdGhtLm5hbWUgaXMgJ0hNQUMnCi1QQVNTIGtleS5hbGdvcml0aG0ubGVuZ3RoIGlz IDE2Ci1QQVNTIGtleS51c2FnZXMgaXMgWyJkZWNyeXB0IiwgImVuY3J5cHQiLCAic2lnbiIsICJ2 ZXJpZnkiXQorRkFJTCBDaGVjayBmYWlsZWQgaW4gd29ya2VyOiBrZXkgaXMgbnVsbAogUEFTUyBz dWNjZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQogCiBURVNUIENPTVBMRVRFCkluZGV4OiBMYXlvdXRU ZXN0cy9jcnlwdG8vc3VidGxlL3Jlc291cmNlcy9wb3N0TWVzc2FnZS13b3JrZXIuanMKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gTGF5b3V0VGVzdHMvY3J5cHRvL3N1YnRsZS9yZXNvdXJjZXMvcG9zdE1lc3NhZ2Ut d29ya2VyLmpzCShyZXZpc2lvbiAxNjM5NDgpCisrKyBMYXlvdXRUZXN0cy9jcnlwdG8vc3VidGxl L3Jlc291cmNlcy9wb3N0TWVzc2FnZS13b3JrZXIuanMJKHdvcmtpbmcgY29weSkKQEAgLTEsNiAr MSw4IEBACiBvbm1lc3NhZ2UgPSBmdW5jdGlvbihldnQpCiB7CiAgICAgdmFyIGtleSA9IGV2dC5k YXRhOworICAgIGlmICgha2V5KQorICAgICAgICBwb3N0TWVzc2FnZSh7IHJlc3VsdDpmYWxzZSwg bWVzc2FnZTona2V5IGlzICcgKyBrZXkgfSk7CiAgICAgaWYgKGtleS50eXBlICE9ICdzZWNyZXQn KQogICAgICAgICBwb3N0TWVzc2FnZSh7IHJlc3VsdDpmYWxzZSwgbWVzc2FnZTona2V5LnR5cGUg c2hvdWxkIGJlICJzZWNyZXQiJyB9KTsKICAgICBlbHNlIGlmICgha2V5LmV4dHJhY3RhYmxlKQpJ bmRleDogTGF5b3V0VGVzdHMvcGxhdGZvcm0vbWFjL1Rlc3RFeHBlY3RhdGlvbnMKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gTGF5b3V0VGVzdHMvcGxhdGZvcm0vbWFjL1Rlc3RFeHBlY3RhdGlvbnMJKHJldmlzaW9u IDE2Mzk1MikKKysrIExheW91dFRlc3RzL3BsYXRmb3JtL21hYy9UZXN0RXhwZWN0YXRpb25zCSh3 b3JraW5nIGNvcHkpCkBAIC0xMjY1LDkgKzEyNjUsNiBAQCBtZWRpYS90cmFjay92aWRlbwogIyBT dWJ0bGVDcnlwdG8gaXMgZGlzYWJsZWQgb24gTW91bnRhaW4gTGlvbgogd2Via2l0Lm9yZy9iLzEy NDI2MSBbIE1vdW50YWluTGlvbiBdIGNyeXB0by9zdWJ0bGUgWyBTa2lwIF0KIAotIyBEZXNlcmlh bGl6YXRpb24gb2YgYSBDcnlwdG9LZXkgaW4gYSB3b3JrZXIgZmFpbHMgd2l0aCBhIGNyYXNoLgot Y3J5cHRvL3N1YnRsZS9wb3N0TWVzc2FnZS13b3JrZXIuaHRtbCBbIFNraXAgXQotCiB3ZWJraXQu b3JnL2IvMTI0MzExIGNvbXBvc2l0aW5nL3JlZ2lvbnMvdHJhbnNmb3JtLXRyYW5zcGFyZW50LXBv c2l0aW9uZWQtdmlkZW8taW5zaWRlLXJlZ2lvbi5odG1sIFsgUGFzcyBJbWFnZU9ubHlGYWlsdXJl IF0KIAogd2Via2l0Lm9yZy9iLzEyNDMxOCBmdWxsc2NyZWVuL2Fub255bW91cy1ibG9jay1tZXJn ZS1jcmFzaC5odG1sIFsgUGFzcyBGYWlsdXJlIF0K