Bug 210816 – Canonicalize JSBigInt generated by structured-cloning by calling rightTrim

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Call rightTrim to ensure that corrupted serialized data should not produce wrong JSBigInt

[Yusuke Suzuki]

Created attachment 397119 [details]
Patch

[Yusuke Suzuki]

Created attachment 397123 [details]
Patch

[Yusuke Suzuki]

Created attachment 397128 [details]
Patch

[Darin Adler]

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.

[Keith Miller]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Darin Adler]

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

[Yusuke Suzuki]

Created attachment 397134 [details]
Patch

[Yusuke Suzuki]

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

[Keith Miller]

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.

[Yusuke Suzuki]

I'll further add a test using `internals.deserializeBuffer` to test that this canonicalization happens correctly!

[Yusuke Suzuki]

Created attachment 397143 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 397145 [details]
Patch for landing

[EWS]

Committed r260489: <https://trac.webkit.org/changeset/260489>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397145 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62148234>