Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1577783004:
[v8] don't crash when ArrayBuffer allocation fails (Closed)
|
Created:
4 years, 11 months ago by caitp (gmail) Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[v8] don't crash when ArrayBuffer allocation fails
When ArrayBufferContents::Allocator fails to allocate a backing store,
return a nullptr so that v8 knows to throw a RangeError (per
http://tc39.github.io/ecma262/#sec-createbytedatablock). This
replaces the current behaviour of failing an assertion and crashing
the process
BUG=536816, v8:4639
R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org
Committed: https://crrev.com/74b20fa86efa112b53c838b88af7c060eb6e6f65
Cr-Commit-Position: refs/heads/master@{#369488}
Patch Set 1 #Patch Set 2 : don't use fast/js/script-tests/*, infra doesn't seem to use them? #Patch Set 3 : Only check exception type, not message #Patch Set 4 : Add a solution which doesn't modify behaviour of existing code, apart from JS TypedArrays #
Total comments: 5
Patch Set 5 : "Reduce duplication" a little bit #Patch Set 6 : Add a little comment explaining the contract #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== Communicate ArrayBufferContents allocation failure to v8 BUG= ========== to ========== Communicate ArrayBufferContents allocation failure to v8 BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + jochen@chromium.org, littledan@chromium.org
Hi, can you take a quick look? Returns 0 for ArrayBuffer allocation failures, rather than failing an assertion. This enables the V8-side code to throw its RangeError exception, as specified. I'm not positive the new js-test is actually being run or not, but there needs to be an embedder-side test for this, because the correct behaviour depends on the allocation hooks installed by the embedder.
Description was changed from ========== Communicate ArrayBufferContents allocation failure to v8 BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError. This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError. This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ==========
So, the exceptions thrown differ a bit between x86 and x64 builds, because of the whole convert-number-to-size_t thing. I don't think this is a big problem, so my preference would be to just not verify the exception message, and instead just look at the fact that it is a range error.
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + haraken@chromium.org
+junov, who might be interested in this.
On 2016/01/12 14:09:29, Stephen White wrote: > +junov, who might be interested in this. not lgtm. This change is a potential security risk because not all users of ArrayBuffer have null pointer checks, which could potentially be exploited to access arbitrary memory addresses. Also, not all uses of ArrayBuffer are in locations from which we can throw (can be outside of the scope of a script execution context), and often there is no graceful failure opportunity, so there are many situations where we'd like to continue having OOM crashes. Each call site needs to be considered carefully. I started working on a full blink-wide solution a couple months ago: https://codereview.chromium.org/1414553002/ The next step in getting that landed is to make an inventory of affected call sites and to identify the follow-up work. I intend to resume and complete that work this quarter. If you want to pitch in, be my guest, contact me off review to coordinate.
jochen@chromium.org changed reviewers: + junov@chromium.org
(just for the record, there's another allocator we use in //gin/array_buffer.cc)
On 2016/01/12 15:51:15, jochen wrote: > (just for the record, there's another allocator we use in //gin/array_buffer.cc) the gin allocator looks fine, as it just defers to libc stuff which should already do the right thing I'd expect that most failures mentioned by junov would be found fairly early on by ASAN fuzzing, but I'm looking for potential failure cases anyways. So far I'm mostly finding safe things which have their own asserts in place
New version doesn't change behaviour of any existing code, but adds a new `allocateNewOrNull()` method helper, used by the V8 bindings to do the right thing. Existing code will continue to rely on the partition allocator's IMMEDIATE_CRASH()
On 2016/01/13 01:13:10, caitp wrote: > New version doesn't change behaviour of any existing code, but adds a new > `allocateNewOrNull()` method helper, used by the V8 bindings to do the right > thing. Existing code will continue to rely on the partition allocator's > IMMEDIATE_CRASH() This is fine by me. And it is probably the incremental approach I should have gone for in the first place instead of the unweildy monolithic change I started. My goal was to remove the old entry point to force all call sites to explicitly chose between "...OrNull" and "...OrCrash". That approach turned into a rabbit hole. I am not an OWNER in wtf or v8, but lgtm (to retract my previous objection)
lgtm, but I am not so convinced that libc/the OS will always give you what you want. Linux itself, for one, is known to OOM rather than return memory allocation failures. https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/ArrayBufferContents.cpp (right): https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:116: } Any way you could reduce the duplication here with the above method, for example make it take flags as an argument?
I'm probably not going to take the time to write a whole new heap allocator for gin, and am not familiar with that part of the codebase at all. Maybe someone else could take a look at it and see how reliable the "return null" behaviour is there? https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/ArrayBufferContents.cpp (right): https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:116: } On 2016/01/13 22:55:14, Dan Ehrenberg wrote: > Any way you could reduce the duplication here with the above method, for example > make it take flags as an argument? done
https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:335: WTF::ArrayBufferContents::allocateMemoryOrNull(size, WTF::ArrayBufferContents::ZeroInitialize, data); I'm a bit confused. If you call allocateMemoryOrNull and take no action when it returns nullptr, you are just ignoring the allocation failure, aren't you?
https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:335: WTF::ArrayBufferContents::allocateMemoryOrNull(size, WTF::ArrayBufferContents::ZeroInitialize, data); On 2016/01/14 01:52:33, haraken wrote: > > I'm a bit confused. If you call allocateMemoryOrNull and take no action when it > returns nullptr, you are just ignoring the allocation failure, aren't you? The ArrayBufferAllocator is used (only) by v8, which throws a RangeError of allocation fails (signaled by returning null here). The idea is to make the Blink ArrayBufferAllocator honor the implicit contract that v8 expects
LGTM https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1577783004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:335: WTF::ArrayBufferContents::allocateMemoryOrNull(size, WTF::ArrayBufferContents::ZeroInitialize, data); On 2016/01/14 01:59:04, caitp wrote: > On 2016/01/14 01:52:33, haraken wrote: > > > > I'm a bit confused. If you call allocateMemoryOrNull and take no action when > it > > returns nullptr, you are just ignoring the allocation failure, aren't you? > > The ArrayBufferAllocator is used (only) by v8, which throws a RangeError of > allocation fails (signaled by returning null here). The idea is to make the > Blink ArrayBufferAllocator honor the implicit contract that v8 expects Thanks, makes sense. Can you add a comment about it?
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, junov@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1577783004/#ps100001 (title: "Add a little comment explaining the contract")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577783004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577783004/100001
Message was sent while issue was closed.
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org ========== to ========== [v8] don't crash when ArrayBuffer allocation fails When ArrayBufferContents::Allocator fails to allocate a backing store, return a nullptr so that v8 knows to throw a RangeError (per http://tc39.github.io/ecma262/#sec-createbytedatablock). This replaces the current behaviour of failing an assertion and crashing the process BUG=536816, v8:4639 R=jochen@chromium.org, littledan@chromium.org, haraken@chromium.org Committed: https://crrev.com/74b20fa86efa112b53c838b88af7c060eb6e6f65 Cr-Commit-Position: refs/heads/master@{#369488} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/74b20fa86efa112b53c838b88af7c060eb6e6f65 Cr-Commit-Position: refs/heads/master@{#369488} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
