IndexedDB: Abort transaction/close connection on value deserialize failure |
||||||
Issue descriptionRight now we DCHECK that serialized values deserialize correctly. We attempt to defend against version skew (i.e. users downgrading, v8 rollbacks) in other ways but as a defense-in-depth we should: When accessing request.result... * Check that deserialization succeeded (deserializeIDBValue) * If it failed, propagate the failure out * Throw (?) in script * Abort the transaction * Force-close the connection This will be unexpected from script (the request.result getter should never throw, but `null` and `undefined` are completely valid results, and we want to signal failure)
,
Mar 22 2017
,
Mar 28 2017
pwnall@ - any update here?
,
Mar 31 2017
Repro steps:
Part A: Obtaining a corrupt database
1) Apply the following diff to src/v8 in the Chromium tree
diff --git a/src/value-serializer.cc b/src/value-serializer.cc
index acedb4be32..d916a60df6 100644
--- a/src/value-serializer.cc
+++ b/src/value-serializer.cc
@@ -29,7 +29,7 @@ namespace internal {
// Version 12: regexp and string objects share normal string encoding
// Version 13: host objects have an explicit tag (rather than handling all
// unknown tags)
-static const uint32_t kLatestVersion = 13;
+static const uint32_t kLatestVersion = 14;
static const int kPretenureThreshold = 100 * KB;
2) Build Chromium (preferably in Release mode, as that will be needed later).
ninja -C out/Release chrome
3) Run the build against the attached file
out/Release/Chromium.app/Contents/MacOS/Chromium file:///Users/pwnall/tmp/b703704.html
(change the command to reflect your OS and homedir)
Part B: Reproducing the crash
1) Revert the diff applied above, or just use a different checkout
cd src/v8
git checkout -- .
cd ../..
2) Build a Release (no DCHECKs) version of Chromium. DCHECKs will trigger a different crash.
ninja -C out/Release chrome
3) Run Chromium against the same (attached) file.
out/Release/Chromium.app/Contents/MacOS/Chromium file:///Users/pwnall/tmp/b703704.html
Note that running a build with DCHECKs triggers the following crash instead:
# Fatal error in ../../v8/src/api.h, line 345
# Check failed: that == __null || (*reinterpret_cast<v8::internal::Object* const*>(that))->IsJSReceiver().
#
0 libbase.dylib 0x000000010b2e523e base::debug::StackTrace::StackTrace(unsigned long) + 174
1 libbase.dylib 0x000000010b2e52dd base::debug::StackTrace::StackTrace(unsigned long) + 29
2 libbase.dylib 0x000000010b2e373c base::debug::StackTrace::StackTrace() + 28
3 libgin.dylib 0x00000001390b40b5 gin::(anonymous namespace)::PrintStackTrace() + 37
4 libv8_libbase.dylib 0x000000013d512c7c V8_Fatal + 220
5 libv8.dylib 0x0000000109ca78f5 v8::Object::CreateDataProperty(v8::Local<v8::Context>, v8::Local<v8::Name>, v8::Local<v8::Value>) + 965
6 libblink_modules.dylib 0x00000001440e3cdd blink::injectV8KeyIntoV8Value(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Value>, blink::IDBKeyPath const&) + 3037
7 libblink_modules.dylib 0x00000001440e59db blink::assertPrimaryKeyValidOrInjectable(blink::ScriptState*, blink::IDBValue const*) + 635
8 libblink_modules.dylib 0x0000000144b654f1 blink::IDBCursor::value(blink::ScriptState*) + 513
9 libblink_modules.dylib 0x0000000144350329 blink::IDBCursorWithValueV8Internal::valueAttributeGetter(v8::FunctionCallbackInfo<v8::Value> const&) + 809
10 libblink_modules.dylib 0x000000014434fff5 blink::V8IDBCursorWithValue::valueAttributeGetterCallback(v8::FunctionCallbackInfo<v8::Value> const&) + 21
11 libv8.dylib 0x0000000109c7c212 v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) + 450
12 libv8.dylib 0x0000000109d8c9f1 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 1169
13 libv8.dylib 0x0000000109d8b756 v8::internal::Builtins::InvokeApiFunction(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::HeapObject>) + 1110
14 libv8.dylib 0x000000010a36511e v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*) + 798
15 libv8.dylib 0x000000010a364434 v8::internal::Object::GetProperty(v8::internal::LookupIterator*) + 260
16 libv8.dylib 0x000000010a28c864 v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>) + 516
17 libv8.dylib 0x000000010a29837f v8::internal::__RT_impl_Runtime_LoadIC_Miss(v8::internal::Arguments, v8::internal::Isolate*) + 287
,
Mar 31 2017
#3: Sorry for the slowness on this issue, I got distracted. The repro is in #4, a CL is in flight at http://crrev.com/2781273004
,
Apr 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdd55622ffec06c7513b366777fe53566aabde4f commit cdd55622ffec06c7513b366777fe53566aabde4f Author: pwnall <pwnall@chromium.org> Date: Wed Apr 05 02:10:50 2017 Graceful handling of new versions of IndexedDB serialized data. The renderer currently crashes when attempting to de-serialize an IndexedDB value whose version number is too new (past what we can read), in a very specific situation -- the object store must have a keyPath and autoIncrement set to true. In all other situatios where IndexedDB value serialization fails due to overly new versions, the resulting value is null. This is not as terrible as it sounds, because a future version would generally indicate that a user switched channels and got an older Chrome build. Random storage corruption results in CRC errors, which are handled differently. This CL fixes the crash, resulting in the same behavior (null values) as in other cases involving data saved by newer versions. TESTED=also verified the fix against the manual bug repro steps BUG=703704 Review-Url: https://codereview.chromium.org/2781273004 Cr-Commit-Position: refs/heads/master@{#461938} [modify] https://crrev.com/cdd55622ffec06c7513b366777fe53566aabde4f/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp [modify] https://crrev.com/cdd55622ffec06c7513b366777fe53566aabde4f/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.h [modify] https://crrev.com/cdd55622ffec06c7513b366777fe53566aabde4f/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp
,
May 18 2017
We're not crashing anymore. This bug isn't fixed yet -- there is remaining work required to turn nulls (which are incorrect behavior, in my understanding of the spec) into exceptions or transaction aborts. However, this work is not P1 and it's less important than other current issues, so I'm opening up this issue.
,
May 18 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 20 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by jsb...@chromium.org
, Mar 21 2017