Bug 128657 – Don't crash when SerializedScriptValue deserialization fails

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 223947 [details]
proposed fix

[Oliver Hunt]

Comment on attachment 223947 [details]
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?

[Alexey Proskuryakov]

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

[Oliver Hunt]

(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

[Oliver Hunt]

Comment on attachment 223947 [details]
proposed fix

r+ based on alexia's comments

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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