Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615803002:
Disallow SharedArrayBuffer in postMessage transfer list (Closed)
DescriptionDisallow SharedArrayBuffer in postMessage transfer list
This is the Blink-side change to match the V8 change here:
https://codereview.chromium.org/2570433005.
BUG=676063
Review-Url: https://codereview.chromium.org/2615803002
Cr-Commit-Position: refs/heads/master@{#442111}
Committed: https://chromium.googlesource.com/chromium/src/+/8b61d57ecb303d484ec2024bee09120c2af4bcf7
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : add test, fix indexing #
Total comments: 6
Patch Set 4 : fixes #
Total comments: 2
Patch Set 5 : remove exceptionState #
Messages
Total messages: 30 (18 generated)
binji@chromium.org changed reviewers: + haraken@chromium.org
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:48: if (!prepareTransfer(transferables, exceptionState)) Instead of using a return value, can we check exceptionState.hadException()? https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:393: if (!V8DOMWrapper::isWrapper(isolate, v8SharedArrayBuffer)) { I think this branch is a bit redundant. If sharedArrayBuffer is null, it already means that the object is not something that can be cloned. Then we can just throw DataCloneError. i.e., line 399 - 403 would not be needed. https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:411: index += m_transferables->arrayBuffers.size(); What is this doing? What happens if some entries are removed from m_sharedArrayBuffers or m_transferables? Won't we end up with returning the same ID for different shared buffers?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:48: if (!prepareTransfer(transferables, exceptionState)) On 2017/01/05 02:02:06, haraken wrote: > > Instead of using a return value, can we check exceptionState.hadException()? Done. https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:393: if (!V8DOMWrapper::isWrapper(isolate, v8SharedArrayBuffer)) { On 2017/01/05 02:02:06, haraken wrote: > > I think this branch is a bit redundant. If sharedArrayBuffer is null, it already > means that the object is not something that can be cloned. Then we can just > throw DataCloneError. > > i.e., line 399 - 403 would not be needed. Done. https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:411: index += m_transferables->arrayBuffers.size(); On 2017/01/05 02:02:06, haraken wrote: > > What is this doing? > > What happens if some entries are removed from m_sharedArrayBuffers or > m_transferables? Won't we end up with returning the same ID for different shared > buffers? There shouldn't be any entries removed from m_sharedArrayBuffers or m_transferables, AFAICT. The idea is that we want to find all the SABs that are found in the message, and add them to the m_sharedArrayBuffers list as side data. The ArrayBufferContents array in the SerializedScriptValue holds both transferred ArrayBuffers and shared SharedArrayBuffers, but I make sure to put all the sharedArrayBuffers after any transferred buffers (see finalizeTransfer above).
LGTM https://codereview.chromium.org/2615803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:402: index += m_transferables->arrayBuffers.size(); Why do you need to add m_transferables->arrayBuffers.size() to index? I guess m_sharedArrayBuffers.size() would be enough to get a unique id. In any case, let's add a comment about why we can get a unique id with the algorithm you implemented.
https://codereview.chromium.org/2615803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:402: index += m_transferables->arrayBuffers.size(); On 2017/01/06 00:12:53, haraken wrote: > > Why do you need to add m_transferables->arrayBuffers.size() to index? I guess > m_sharedArrayBuffers.size() would be enough to get a unique id. Whoops, I made a mistake here! Thanks :) > > In any case, let's add a comment about why we can get a unique id with the > algorithm you implemented. Done.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
jbroman@chromium.org changed reviewers: + jbroman@chromium.org
https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:393: if (!V8DOMWrapper::isWrapper(isolate, v8SharedArrayBuffer)) { On 2017/01/05 at 02:02:06, haraken wrote: > I think this branch is a bit redundant. If sharedArrayBuffer is null, it already means that the object is not something that can be cloned. Then we can just throw DataCloneError. > > i.e., line 399 - 403 would not be needed. +1 I would be rather astonished to find that there is some type of Blink object that is wrapped by v8::SharedArrayBuffer and yet is not DOMSharedArrayBuffer. https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92: } I think you should return after throwing this exception, rather than continuing to iterate. https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:289: if (wrapperTypeInfo == &V8SharedArrayBuffer::wrapperTypeInfo) { Can this happen? I don't see how something with JS_ARRAY_BUFFER_TYPE should be able to get here (and if it does, I'd expect the default error message to be fine). https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:390: V8SharedArrayBuffer::toImplWithTypeCheck(isolate, v8SharedArrayBuffer); How can this possibly fail? This should succeed as long as v8SharedArrayBuffer->IsSharedArrayBuffer(). But that must be true unless a bad type-cast has already happened somewhere else. I think this can just be toImpl.
https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92: } On 2017/01/06 16:22:02, jbroman wrote: > I think you should return after throwing this exception, rather than continuing > to iterate. Done. https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:289: if (wrapperTypeInfo == &V8SharedArrayBuffer::wrapperTypeInfo) { On 2017/01/06 16:22:02, jbroman wrote: > Can this happen? I don't see how something with JS_ARRAY_BUFFER_TYPE should be > able to get here (and if it does, I'd expect the default error message to be > fine). Oops, yeah I think you're right. https://codereview.chromium.org/2615803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:390: V8SharedArrayBuffer::toImplWithTypeCheck(isolate, v8SharedArrayBuffer); On 2017/01/06 16:22:02, jbroman wrote: > How can this possibly fail? This should succeed as long as > v8SharedArrayBuffer->IsSharedArrayBuffer(). But that must be true unless a bad > type-cast has already happened somewhere else. I think this can just be toImpl. Ah, I think this was a hold-over from when the type was v8::Local<v8::Object>.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with one remaining comment https://codereview.chromium.org/2615803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:387: if (!sharedArrayBuffer) { I don't think V8SharedArrayBuffer::toImpl can return null, so this if statement can go away. With that gone, I don't think there's any way for this to fail, right? If so then we can also do away with the ExceptionState.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2615803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp (right): https://codereview.chromium.org/2615803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:387: if (!sharedArrayBuffer) { On 2017/01/06 20:00:34, jbroman wrote: > I don't think V8SharedArrayBuffer::toImpl can return null, so this if statement > can go away. > > With that gone, I don't think there's any way for this to fail, right? If so > then we can also do away with the ExceptionState. Done.
The CQ bit was checked by binji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2615803002/#ps80001 (title: "remove exceptionState")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483742937699780,
"parent_rev": "eb9098d5864f531fdc7d72550c4928daed0e626b", "commit_rev":
"8b61d57ecb303d484ec2024bee09120c2af4bcf7"}
Message was sent while issue was closed.
Description was changed from ========== Disallow SharedArrayBuffer in postMessage transfer list This is the Blink-side change to match the V8 change here: https://codereview.chromium.org/2570433005. BUG=676063 ========== to ========== Disallow SharedArrayBuffer in postMessage transfer list This is the Blink-side change to match the V8 change here: https://codereview.chromium.org/2570433005. BUG=676063 Review-Url: https://codereview.chromium.org/2615803002 Cr-Commit-Position: refs/heads/master@{#442111} Committed: https://chromium.googlesource.com/chromium/src/+/8b61d57ecb303d484ec2024bee09... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8b61d57ecb303d484ec2024bee09... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
