IndexedDB: Enforce unsigned long/unsigned long long ranges
authorjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Oct 2012 22:48:44 +0000 (22:48 +0000)
committerjsbell@chromium.org <jsbell@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Oct 2012 22:48:44 +0000 (22:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=99637

Reviewed by Tony Chang.

Source/WebCore:

The IndexedDB spec has [EnforceRange] specified on unsigned long and unsigned long long
arguments, which requires the implementation to throw TypeError for negative values or
values that exceed 2^53-1 (maximum JS number that behaves like an integer) - and 0 is
specifically forbidden by the APIs as well.

A more correct fix in the binding layer is in webkit.org/b/96798 but we can temporarily
address this in the implementation.

Also refactor to prevent IDBFactory.open(name, -1) from triggering an internal code path.

Tests: storage/indexeddb/cursor-advance.html
       storage/indexeddb/intversion-bad-parameters.html
       storage/indexeddb/intversion-encoding.html

* Modules/indexeddb/IDBCursor.cpp:
(WebCore::IDBCursor::advance): Validate argument range.
* Modules/indexeddb/IDBCursor.h:
(IDBCursor):
* Modules/indexeddb/IDBCursor.idl: Drop "unsigned" qualifier as the binding code is
not yet doing the correct validation.
* Modules/indexeddb/IDBFactory.cpp: Refactor to prevent open(name, -1)
(WebCore):
(WebCore::IDBFactory::open): Validate the int version here, then pass to...
(WebCore::IDBFactory::openInternal): ... this method.
* Modules/indexeddb/IDBFactory.h:
(IDBFactory):
* Modules/indexeddb/IDBFactory.idl: Drop "unsigned" qualifier; meaningless to binding
code right now, can be re-added once webkit.org/b/96798 lands.

LayoutTests:

Additional edge case tests and updated expectations.

* storage/indexeddb/cursor-advance-expected.txt:
* storage/indexeddb/intversion-bad-parameters-expected.txt:
* storage/indexeddb/intversion-encoding-expected.txt:
* storage/indexeddb/resources/cursor-advance.js:
(testBadAdvance.advanceBadly):
(testBadAdvance):
* storage/indexeddb/resources/intversion-bad-parameters.js:
(deleteSuccess):
* storage/indexeddb/resources/intversion-encoding.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@131658 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/storage/indexeddb/cursor-advance-expected.txt
LayoutTests/storage/indexeddb/intversion-bad-parameters-expected.txt
LayoutTests/storage/indexeddb/intversion-encoding-expected.txt
LayoutTests/storage/indexeddb/open-bad-versions-expected.txt [new file with mode: 0644]
LayoutTests/storage/indexeddb/open-bad-versions.html [new file with mode: 0644]
LayoutTests/storage/indexeddb/resources/cursor-advance.js
LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js
LayoutTests/storage/indexeddb/resources/intversion-encoding.js
Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBCursor.cpp
Source/WebCore/Modules/indexeddb/IDBCursor.h
Source/WebCore/Modules/indexeddb/IDBCursor.idl
Source/WebCore/Modules/indexeddb/IDBFactory.cpp
Source/WebCore/Modules/indexeddb/IDBFactory.h
Source/WebCore/Modules/indexeddb/IDBFactory.idl

index d8070a4..b2914cc 100644 (file)
@@ -1,3 +1,22 @@
+2012-10-17  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Enforce unsigned long/unsigned long long ranges
+        https://bugs.webkit.org/show_bug.cgi?id=99637
+
+        Reviewed by Tony Chang.
+
+        Additional edge case tests and updated expectations.
+
+        * storage/indexeddb/cursor-advance-expected.txt:
+        * storage/indexeddb/intversion-bad-parameters-expected.txt:
+        * storage/indexeddb/intversion-encoding-expected.txt:
+        * storage/indexeddb/resources/cursor-advance.js:
+        (testBadAdvance.advanceBadly):
+        (testBadAdvance):
+        * storage/indexeddb/resources/intversion-bad-parameters.js:
+        (deleteSuccess):
+        * storage/indexeddb/resources/intversion-encoding.js:
+
 2012-10-17  Tony Chang  <tony@chromium.org>
 
         fast/forms/range/input-appearance-range-rtl.html off by one pixel
index 45efbd7..7aa6f71 100644 (file)
@@ -137,6 +137,12 @@ request = objectStore.openCursor()
 Expecting TypeError exception from cursor.advance(0)
 PASS Exception was thrown.
 PASS cursor.advance(0) threw TypeError: Type error
+Expecting TypeError exception from cursor.advance(-1)
+PASS Exception was thrown.
+PASS cursor.advance(-1) threw TypeError: Type error
+Expecting TypeError exception from cursor.advance(0x20000000000000)
+PASS Exception was thrown.
+PASS cursor.advance(0x20000000000000) threw TypeError: Type error
 testDelete()
 trans = db.transaction(objectStoreName, 'readwrite')
 objectStore = trans.objectStore(objectStoreName)
index e55fd3c..dac686b 100644 (file)
@@ -18,11 +18,12 @@ PASS indexedDB.open(dbname, 0) threw TypeError: Type error
 Expecting TypeError exception from indexedDB.open(dbname, -5)
 PASS Exception was thrown.
 PASS indexedDB.open(dbname, -5) threw TypeError: Type error
-
-FIXME: Using -1 doesn't throw TypeError but it should
-Expecting TypeError exception from request = indexedDB.open(dbname, -1)
-FAIL No exception thrown!
-FAIL onupgradeneeded called unexpectedly
+Expecting TypeError exception from indexedDB.open(dbname, -1)
+PASS Exception was thrown.
+PASS indexedDB.open(dbname, -1) threw TypeError: Type error
+Expecting TypeError exception from indexedDB.open(dbname, 0x20000000000000)
+PASS Exception was thrown.
+PASS indexedDB.open(dbname, 0x20000000000000) threw TypeError: Type error
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 1d50b77..467d141 100644 (file)
@@ -81,7 +81,7 @@ PASS db.version is version
 db.close()
 
 openFirstTime():
-version = 9007199254740992
+version = 9007199254740991
 upgradeNeededFired = false
 request = indexedDB.open(dbname, version)
 upgradeNeededFired = true
diff --git a/LayoutTests/storage/indexeddb/open-bad-versions-expected.txt b/LayoutTests/storage/indexeddb/open-bad-versions-expected.txt
new file mode 100644 (file)
index 0000000..61cd633
--- /dev/null
@@ -0,0 +1,12 @@
+No crashes when opening and reloading with bad version numbers
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
+
+dbname = String(window.location)
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/indexeddb/open-bad-versions.html b/LayoutTests/storage/indexeddb/open-bad-versions.html
new file mode 100644 (file)
index 0000000..3e371b6
--- /dev/null
@@ -0,0 +1,34 @@
+<html>
+<head>
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+<script src="resources/shared.js"></script>
+</head>
+<body>
+<script>
+
+description("No crashes when opening and reloading with bad version numbers");
+
+function test()
+{
+  removeVendorPrefixes();
+  evalAndLog("dbname = String(window.location)");
+
+  try { indexedDB.open(dbname, -1); } catch (e) { }
+  indexedDB.open(dbname, 2);
+  try { indexedDB.open(dbname, -1); } catch (e) { }
+
+  setTimeout(function() {
+    if (!window.location.search) {
+      window.location = window.location + "?2";
+    } else {
+      finishJSTest();
+    }
+  }, 500);
+}
+
+test();
+
+</script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
index 3f4f52d..20011b8 100644 (file)
@@ -330,6 +330,8 @@ function testBadAdvance()
         cursor = event.target.result;
 
         evalAndExpectExceptionClass("cursor.advance(0)", "TypeError");
+        evalAndExpectExceptionClass("cursor.advance(-1)", "TypeError");
+        evalAndExpectExceptionClass("cursor.advance(0x20000000000000)", "TypeError");
         testDelete();
     }
     request.onsuccess = advanceBadly;
index 1988a3d..b9f35d0 100644 (file)
@@ -20,13 +20,9 @@ function deleteSuccess(evt) {
     evalAndExpectExceptionClass("indexedDB.open(dbname, 'stringversion')", "TypeError");
     evalAndExpectExceptionClass("indexedDB.open(dbname, 0)", "TypeError");
     evalAndExpectExceptionClass("indexedDB.open(dbname, -5)", "TypeError");
-    debug("");
-    debug("FIXME: Using -1 doesn't throw TypeError but it should");
-    evalAndExpectExceptionClass("request = indexedDB.open(dbname, -1)", "TypeError");
-    request.onsuccess = unexpectedSuccessCallback;
-    request.onerror = unexpectedErrorCallback;
-    request.onblocked = unexpectedBlockedCallback;
-    request.onupgradeneeded = unexpectedUpgradeNeededCallback;
+    evalAndExpectExceptionClass("indexedDB.open(dbname, -1)", "TypeError");
+    evalAndExpectExceptionClass("indexedDB.open(dbname, 0x20000000000000)", "TypeError");
+    finishJSTest();
 }
 
 test();
index e583fc6..65a228c 100644 (file)
@@ -9,7 +9,7 @@ versions = [1,
             0x7f,
             0x80,
             0x80000000,
-            9007199254740992]; // 2^53, maximum JavaScript integer.
+            9007199254740991]; // 2^53-1, maximum JavaScript integer.
 
 function test()
 {
index 5065d8b..431616f 100644 (file)
@@ -1,3 +1,39 @@
+2012-10-17  Joshua Bell  <jsbell@chromium.org>
+
+        IndexedDB: Enforce unsigned long/unsigned long long ranges
+        https://bugs.webkit.org/show_bug.cgi?id=99637
+
+        Reviewed by Tony Chang.
+
+        The IndexedDB spec has [EnforceRange] specified on unsigned long and unsigned long long
+        arguments, which requires the implementation to throw TypeError for negative values or
+        values that exceed 2^53-1 (maximum JS number that behaves like an integer) - and 0 is
+        specifically forbidden by the APIs as well.
+
+        A more correct fix in the binding layer is in webkit.org/b/96798 but we can temporarily
+        address this in the implementation.
+
+        Also refactor to prevent IDBFactory.open(name, -1) from triggering an internal code path.
+
+        Tests: storage/indexeddb/cursor-advance.html
+               storage/indexeddb/intversion-bad-parameters.html
+               storage/indexeddb/intversion-encoding.html
+
+        * Modules/indexeddb/IDBCursor.cpp:
+        (WebCore::IDBCursor::advance): Validate argument range.
+        * Modules/indexeddb/IDBCursor.h:
+        (IDBCursor):
+        * Modules/indexeddb/IDBCursor.idl: Drop "unsigned" qualifier as the binding code is
+        not yet doing the correct validation.
+        * Modules/indexeddb/IDBFactory.cpp: Refactor to prevent open(name, -1)
+        (WebCore):
+        (WebCore::IDBFactory::open): Validate the int version here, then pass to...
+        (WebCore::IDBFactory::openInternal): ... this method.
+        * Modules/indexeddb/IDBFactory.h:
+        (IDBFactory):
+        * Modules/indexeddb/IDBFactory.idl: Drop "unsigned" qualifier; meaningless to binding
+        code right now, can be re-added once webkit.org/b/96798 lands.
+
 2012-10-17  Tony Chang  <tony@chromium.org>
 
         fast/forms/range/input-appearance-range-rtl.html off by one pixel
index dc33ec2..990e3a4 100644 (file)
@@ -156,7 +156,7 @@ PassRefPtr<IDBRequest> IDBCursor::update(ScriptExecutionContext* context, Script
     return objectStore->put(IDBObjectStoreBackendInterface::CursorUpdate, IDBAny::create(this), context, value, m_currentPrimaryKey, ec);
 }
 
-void IDBCursor::advance(unsigned long count, ExceptionCode& ec)
+void IDBCursor::advance(long count, ExceptionCode& ec)
 {
     IDB_TRACE("IDBCursor::advance");
     if (!m_gotValue) {
@@ -169,7 +169,9 @@ void IDBCursor::advance(unsigned long count, ExceptionCode& ec)
         return;
     }
 
-    if (!count) {
+    // FIXME: This should only need to check for 0 once webkit.org/b/96798 lands.
+    const int64_t maxECMAScriptInteger = 0x20000000000000LL - 1;
+    if (count < 1 || count > maxECMAScriptInteger) {
         ec = NATIVE_TYPE_ERR;
         return;
     }
index a057686..fe725c9 100644 (file)
@@ -75,7 +75,7 @@ public:
     IDBAny* source() const;
 
     PassRefPtr<IDBRequest> update(ScriptExecutionContext*, ScriptValue&, ExceptionCode&);
-    void advance(unsigned long, ExceptionCode&);
+    void advance(long, ExceptionCode&);
     void continueFunction(PassRefPtr<IDBKey>, ExceptionCode&);
     PassRefPtr<IDBRequest> deleteFunction(ScriptExecutionContext*, ExceptionCode&);
 
index 375479b..b514fe9 100644 (file)
@@ -39,7 +39,8 @@
 
     [CallWith=ScriptExecutionContext] IDBRequest update(in any value)
         raises (IDBDatabaseException);
-    void advance(in unsigned long count)
+    // FIXME: Make this [EnforceRange] unsigned long once webkit.org/b/96798 lands.
+    void advance(in long count)
         raises (IDBDatabaseException);
     [ImplementedAs=continueFunction] void continue(in [Optional] IDBKey key)
         raises (IDBDatabaseException);
index f5fccb4..8535757 100644 (file)
@@ -108,13 +108,19 @@ PassRefPtr<IDBRequest> IDBFactory::getDatabaseNames(ScriptExecutionContext* cont
 
 PassRefPtr<IDBOpenDBRequest> IDBFactory::open(ScriptExecutionContext* context, const String& name, int64_t version, ExceptionCode& ec)
 {
-    if (name.isNull()) {
+    // FIXME: This should only need to check for 0 once webkit.org/b/96798 lands.
+    const int64_t maxECMAScriptInteger = 0x20000000000000LL - 1;
+    if (version < 1 || version > maxECMAScriptInteger) {
         ec = NATIVE_TYPE_ERR;
         return 0;
     }
-    // FIXME: We need to throw an error if script passes -1. Somehow refactor
-    // this to avoid wanting to throw an error with the sentinel.
-    if (!version || version < IDBDatabaseMetadata::NoIntVersion) {
+    return openInternal(context, name, version, ec);
+}
+
+PassRefPtr<IDBOpenDBRequest> IDBFactory::openInternal(ScriptExecutionContext* context, const String& name, int64_t version, ExceptionCode& ec)
+{
+    ASSERT(version >= 1 || version == IDBDatabaseMetadata::NoIntVersion);
+    if (name.isNull()) {
         ec = NATIVE_TYPE_ERR;
         return 0;
     }
@@ -129,7 +135,7 @@ PassRefPtr<IDBOpenDBRequest> IDBFactory::open(ScriptExecutionContext* context, c
 
 PassRefPtr<IDBOpenDBRequest> IDBFactory::open(ScriptExecutionContext* context, const String& name, ExceptionCode& ec)
 {
-    return open(context, name, IDBDatabaseMetadata::NoIntVersion, ec);
+    return openInternal(context, name, IDBDatabaseMetadata::NoIntVersion, ec);
 }
 
 PassRefPtr<IDBVersionChangeRequest> IDBFactory::deleteDatabase(ScriptExecutionContext* context, const String& name, ExceptionCode& ec)
index 7cd961b..4219157 100644 (file)
@@ -67,6 +67,8 @@ public:
 private:
     IDBFactory(IDBFactoryBackendInterface*);
 
+    PassRefPtr<IDBOpenDBRequest> openInternal(ScriptExecutionContext*, const String& name, int64_t version, ExceptionCode&);
+
     RefPtr<IDBFactoryBackendInterface> m_backend;
 };
 
index dfb2768..a24cd2b 100644 (file)
@@ -28,7 +28,8 @@
 ] interface IDBFactory {
     [CallWith=ScriptExecutionContext, ImplementedAs=getDatabaseNames] IDBRequest webkitGetDatabaseNames();
 
-    [CallWith=ScriptExecutionContext] IDBOpenDBRequest open(in DOMString name, in [Optional] unsigned long long version)
+    // FIXME: Make this [EnforceRange] unsigned long long once webkit.org/b/96798 lands.
+    [CallWith=ScriptExecutionContext] IDBOpenDBRequest open(in DOMString name, in [Optional] long long version)
         raises (IDBDatabaseException);
     [CallWith=ScriptExecutionContext] IDBVersionChangeRequest deleteDatabase(in DOMString name)
         raises (IDBDatabaseException);