New issue
Advanced search Search tips

Issue 703704 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Participants' hotlists:
IDB-Stability


Sign in to add a comment

IndexedDB: Abort transaction/close connection on value deserialize failure

Project Member Reported by jsb...@chromium.org, Mar 21 2017

Issue description

Right 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)

 

Comment 1 by jsb...@chromium.org, Mar 21 2017

See issue 698214 and  issue 700603  for context.

Comment 2 by jsb...@chromium.org, Mar 22 2017

Owner: pwnall@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by jsb...@chromium.org, Mar 28 2017

pwnall@ - any update here?

Comment 4 by pwnall@chromium.org, Mar 31 2017

Status: Started (was: Assigned)
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
b703704.html
650 bytes View Download

Comment 5 by pwnall@chromium.org, 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
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by pwnall@chromium.org, May 18 2017

Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Started)
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.
Project Member

Comment 8 by sheriffbot@chromium.org, May 18 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
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

Comment 9 by pwnall@chromium.org, Jun 20 2018

Status: Available (was: Untriaged)

Sign in to add a comment