[JSC] Call rightTrim to ensure that corrupted serialized data should not produce wrong JSBigInt
Created attachment 397119 [details] Patch
Created attachment 397123 [details] Patch
Created attachment 397128 [details] Patch
Comment on attachment 397128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397128&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3038 > + bigInt = bigInt->rightTrim(m_lexicalGlobalObject->vm()); Wouldn’t it be better for deserialization to *fail* if rightTrim would do anything here? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3074 > + bigInt = bigInt->rightTrim(m_lexicalGlobalObject->vm()); Ditto.
Comment on attachment 397128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397128&action=review > Source/JavaScriptCore/ChangeLog:3 > + Call rightTrim to ensure that corrupted serialized data should not produce wrong JSBigInt to make structured-cloning for JSBigInt more robust That's a mouthful, can we just say: "Make structured-cloning for JSBigInt more robust by calling rightTrim"? We can explain why in the ChangLog. > Source/WebCore/ChangeLog:8 > + Let's assume that the serialized data is corrupted. In that case, we should (1) throw an failure if it is detected or (2) produce a right value if it is not detected. For (2) do you mean a valid value? What would the "right" value be for corrupted input? > Source/WebCore/ChangeLog:9 > + We should call `JSBigInt::rightTrim(VM&)` when finishing JSBigInt deserialization to at least ensure that (2). Nit: ensure that (2) => ensure (2) >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3038 >> + bigInt = bigInt->rightTrim(m_lexicalGlobalObject->vm()); > > Wouldn’t it be better for deserialization to *fail* if rightTrim would do anything here? The only upside I can see of not failing would be that maybe a developer could work around a bug on our end. Overall, I agree with you though.
Comment on attachment 397128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397128&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:3 >> + Call rightTrim to ensure that corrupted serialized data should not produce wrong JSBigInt to make structured-cloning for JSBigInt more robust > > That's a mouthful, can we just say: "Make structured-cloning for JSBigInt more robust by calling rightTrim"? > > We can explain why in the ChangLog. Will fix. >> Source/WebCore/ChangeLog:8 >> + Let's assume that the serialized data is corrupted. In that case, we should (1) throw an failure if it is detected or (2) produce a right value if it is not detected. > > For (2) do you mean a valid value? What would the "right" value be for corrupted input? Valid value. Corrupt means that it is not ensuring JSBigInt's data layout invariant. For example, zero should be length = 0 and sign = false. But we can represent this value in various form like, length = 1 and value = 0. Or sign = true, length = 0. This patch is generating correct value while ignoring data layout invariant in JSBigInt. >> Source/WebCore/ChangeLog:9 >> + We should call `JSBigInt::rightTrim(VM&)` when finishing JSBigInt deserialization to at least ensure that (2). > > Nit: ensure that (2) => ensure (2) Fixed. >>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3038 >>> + bigInt = bigInt->rightTrim(m_lexicalGlobalObject->vm()); >> >> Wouldn’t it be better for deserialization to *fail* if rightTrim would do anything here? > > The only upside I can see of not failing would be that maybe a developer could work around a bug on our end. Overall, I agree with you though. My wording would be misleading. Here, what I said wrong is "wrong in terms of internal invariant of JSBigInt". For example, in this format, we have several ways to represent zero JSBigInt. One is 0 length. One is 1 length, and value is zero. One is 0-length, but sign is true. But in JSBigInt layout, we are assuming some invariant in JSBigInt. For example, if JSBigInt is zero, length should be zero, and sign flag should be false. This `rightTrim(VM&)` offers the way to canonicalize JSBigInt format even if the input is not holding JSBigInt's internal invariant. I think this direction is better. We should not expose such an internal invariant of JSBigInt in serialization code. This makes JSBigInt implementation free from the format of serialization code. Just calling rightTrim correctly canonicalize JSBigInt's internal representation regardless of how it is constructed.
Comment on attachment 397128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397128&action=review >>>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3038 >>>> + bigInt = bigInt->rightTrim(m_lexicalGlobalObject->vm()); >>> >>> Wouldn’t it be better for deserialization to *fail* if rightTrim would do anything here? >> >> The only upside I can see of not failing would be that maybe a developer could work around a bug on our end. Overall, I agree with you though. > > My wording would be misleading. Here, what I said wrong is "wrong in terms of internal invariant of JSBigInt". For example, in this format, we have several ways to represent zero JSBigInt. One is 0 length. One is 1 length, and value is zero. One is 0-length, but sign is true. > But in JSBigInt layout, we are assuming some invariant in JSBigInt. For example, if JSBigInt is zero, length should be zero, and sign flag should be false. > This `rightTrim(VM&)` offers the way to canonicalize JSBigInt format even if the input is not holding JSBigInt's internal invariant. > I think this direction is better. We should not expose such an internal invariant of JSBigInt in serialization code. This makes JSBigInt implementation free from the format of serialization code. > Just calling rightTrim correctly canonicalize JSBigInt's internal representation regardless of how it is constructed. Let's consider about adding a new invariant into JSBigInt's internal format. If we throw an error if serialized format does not meet JSBigInt's internal invariant, then, we cannot easily modify JSBigInt's invariant because this serialization format requires backward-compatibility. On the other hand, if this canonicalizes the generated JSBigInt, then we can freely add new invariants into JSBigInt implementation without considering the serialization format.
(In reply to Yusuke Suzuki from comment #6) > I think this direction is better. We should not expose such an internal > invariant of JSBigInt in serialization code. OK.
Created attachment 397134 [details] Patch
(In reply to Darin Adler from comment #8) > (In reply to Yusuke Suzuki from comment #6) > > I think this direction is better. We should not expose such an internal > > invariant of JSBigInt in serialization code. > > OK. Thanks!! I've updated.
Comment on attachment 397134 [details] Patch r=me. I was forgetting that the point of this is to be written to disk. This patch makes sense now.
I'll further add a test using `internals.deserializeBuffer` to test that this canonicalization happens correctly!
Created attachment 397143 [details] Patch for landing
Created attachment 397145 [details] Patch for landing
Committed r260489: <https://trac.webkit.org/changeset/260489> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397145 [details].
<rdar://problem/62148234>