- Revision
- 292822
- Author
- kocsen_ch...@apple.com
- Date
- 2022-04-13 12:53:16 -0700 (Wed, 13 Apr 2022)
Log Message
Cherry-pick r292721. rdar://problem/88249235
Fix size computation in WebCore::StorageMap
https://bugs.webkit.org/show_bug.cgi?id=239024
rdar://88249235
Reviewed by Chris Dumez.
Source/WebCore:
We use currentSize to track size for StorageMap. There are a few issues in current implementation that can make
currentSize incorrect and may lead to overflow:
1. When computing size of key, StorageMap uses parameter key instead of stored key. The problem is that
two Strings can be evaluated to equal while their sizeInBytes() value is different, when one String is 8-bit and
the other is 16-bit. That means removeItem() may decrease currentSize by wrong number (e.g setItem() with an
8-bit key, converting the key to 16-bit, removeItem() with the key). To fix this, StorageMap now always uses
stored key for computation.
2. When map.take(key) or map.get(key) returns null string, StorageMap takes it as the key does not exist and
will not correctly update currentSize, but user of WebCore::StorageMap may store null string as value. To fix
this, StorageMap now check if key exists with find() function.
3. StorageMap only uses CheckedUint32 in setItem(), but removeItem() and importItem() may cause overflow in
currentSize as mentioned above, and the error will not be caught until setItem() is called. To fix this,
StorageMap now uses CheckedUint32 in all places that update currentSize.
New test: WKWebView.LocalStorageNoSizeOverflow
* storage/StorageMap.cpp:
(WebCore::StorageMap::setItem):
(WebCore::StorageMap::removeItem):
(WebCore::StorageMap::importItems):
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
(TEST):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292721 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-613.2.6.0-branch/Source/WebCore/ChangeLog (292821 => 292822)
--- branches/safari-613.2.6.0-branch/Source/WebCore/ChangeLog 2022-04-13 19:43:31 UTC (rev 292821)
+++ branches/safari-613.2.6.0-branch/Source/WebCore/ChangeLog 2022-04-13 19:53:16 UTC (rev 292822)
@@ -1,3 +1,73 @@
+2022-04-13 Kocsen Chung <kocsen_ch...@apple.com>
+
+ Cherry-pick r292721. rdar://problem/88249235
+
+ Fix size computation in WebCore::StorageMap
+ https://bugs.webkit.org/show_bug.cgi?id=239024
+ rdar://88249235
+
+ Reviewed by Chris Dumez.
+
+ Source/WebCore:
+
+ We use currentSize to track size for StorageMap. There are a few issues in current implementation that can make
+ currentSize incorrect and may lead to overflow:
+ 1. When computing size of key, StorageMap uses parameter key instead of stored key. The problem is that
+ two Strings can be evaluated to equal while their sizeInBytes() value is different, when one String is 8-bit and
+ the other is 16-bit. That means removeItem() may decrease currentSize by wrong number (e.g setItem() with an
+ 8-bit key, converting the key to 16-bit, removeItem() with the key). To fix this, StorageMap now always uses
+ stored key for computation.
+ 2. When map.take(key) or map.get(key) returns null string, StorageMap takes it as the key does not exist and
+ will not correctly update currentSize, but user of WebCore::StorageMap may store null string as value. To fix
+ this, StorageMap now check if key exists with find() function.
+ 3. StorageMap only uses CheckedUint32 in setItem(), but removeItem() and importItem() may cause overflow in
+ currentSize as mentioned above, and the error will not be caught until setItem() is called. To fix this,
+ StorageMap now uses CheckedUint32 in all places that update currentSize.
+
+ New test: WKWebView.LocalStorageNoSizeOverflow
+
+ * storage/StorageMap.cpp:
+ (WebCore::StorageMap::setItem):
+ (WebCore::StorageMap::removeItem):
+ (WebCore::StorageMap::importItems):
+
+ Tools:
+
+ * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
+ (TEST):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292721 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-04-11 Sihui Liu <sihui_...@apple.com>
+
+ Fix size computation in WebCore::StorageMap
+ https://bugs.webkit.org/show_bug.cgi?id=239024
+ rdar://88249235
+
+ Reviewed by Chris Dumez.
+
+ We use currentSize to track size for StorageMap. There are a few issues in current implementation that can make
+ currentSize incorrect and may lead to overflow:
+ 1. When computing size of key, StorageMap uses parameter key instead of stored key. The problem is that
+ two Strings can be evaluated to equal while their sizeInBytes() value is different, when one String is 8-bit and
+ the other is 16-bit. That means removeItem() may decrease currentSize by wrong number (e.g setItem() with an
+ 8-bit key, converting the key to 16-bit, removeItem() with the key). To fix this, StorageMap now always uses
+ stored key for computation.
+ 2. When map.take(key) or map.get(key) returns null string, StorageMap takes it as the key does not exist and
+ will not correctly update currentSize, but user of WebCore::StorageMap may store null string as value. To fix
+ this, StorageMap now check if key exists with find() function.
+ 3. StorageMap only uses CheckedUint32 in setItem(), but removeItem() and importItem() may cause overflow in
+ currentSize as mentioned above, and the error will not be caught until setItem() is called. To fix this,
+ StorageMap now uses CheckedUint32 in all places that update currentSize.
+
+ New test: WKWebView.LocalStorageNoSizeOverflow
+
+ * storage/StorageMap.cpp:
+ (WebCore::StorageMap::setItem):
+ (WebCore::StorageMap::removeItem):
+ (WebCore::StorageMap::importItems):
+
2022-04-07 Alan Coon <alanc...@apple.com>
Cherry-pick r291629. rdar://problem/91446360
Modified: branches/safari-613.2.6.0-branch/Source/WebCore/storage/StorageMap.cpp (292821 => 292822)
--- branches/safari-613.2.6.0-branch/Source/WebCore/storage/StorageMap.cpp 2022-04-13 19:43:31 UTC (rev 292821)
+++ branches/safari-613.2.6.0-branch/Source/WebCore/storage/StorageMap.cpp 2022-04-13 19:53:16 UTC (rev 292822)
@@ -88,30 +88,29 @@
void StorageMap::setItem(const String& key, const String& value, String& oldValue, bool& quotaException)
{
ASSERT(!value.isNull());
+
quotaException = false;
-
- // Implement copy-on-write semantics.
- if (m_impl->refCount() > 1)
- m_impl = m_impl->copy();
-
- oldValue = m_impl->map.get(key);
-
- // Quota tracking. This is done in a couple of steps to keep the overflow tracking simple.
CheckedUint32 newSize = m_impl->currentSize;
- newSize -= oldValue.sizeInBytes();
+ auto iter = m_impl->map.find(key);
+ if (iter != m_impl->map.end()) {
+ oldValue = iter->value;
+ newSize -= oldValue.sizeInBytes();
+ } else {
+ oldValue = nullString();
+ newSize += key.sizeInBytes();
+ }
newSize += value.sizeInBytes();
- if (oldValue.isNull())
- newSize += key.sizeInBytes();
if (m_quotaSize != noQuota && (newSize.hasOverflowed() || newSize > m_quotaSize)) {
quotaException = true;
return;
}
- m_impl->currentSize = newSize;
- HashMap<String, String>::AddResult addResult = m_impl->map.add(key, value);
- if (!addResult.isNewEntry)
- addResult.iterator->value = value;
+ // Implement copy-on-write semantics.
+ if (m_impl->refCount() > 1)
+ m_impl = m_impl->copy();
+ m_impl->map.set(key, value);
+ m_impl->currentSize = newSize;
invalidateIterator();
}
@@ -127,19 +126,21 @@
void StorageMap::removeItem(const String& key, String& oldValue)
{
+ oldValue = nullString();
+ CheckedUint32 newSize = m_impl->currentSize;
+ auto iter = m_impl->map.find(key);
+ if (iter == m_impl->map.end())
+ return;
+ oldValue = iter->value;
+ newSize = newSize - iter->key.sizeInBytes() - oldValue.sizeInBytes();
+
// Implement copy-on-write semantics.
if (m_impl->refCount() > 1)
m_impl = m_impl->copy();
- oldValue = m_impl->map.take(key);
- if (oldValue.isNull())
- return;
-
+ m_impl->map.remove(iter);
+ m_impl->currentSize = newSize;
invalidateIterator();
- ASSERT(m_impl->currentSize - key.sizeInBytes() <= m_impl->currentSize);
- m_impl->currentSize -= key.sizeInBytes();
- ASSERT(m_impl->currentSize - oldValue.sizeInBytes() <= m_impl->currentSize);
- m_impl->currentSize -= oldValue.sizeInBytes();
}
void StorageMap::clear()
@@ -160,26 +161,24 @@
void StorageMap::importItems(HashMap<String, String>&& items)
{
+ CheckedUint32 newSize = m_impl->currentSize;
if (m_impl->map.isEmpty()) {
- // Fast path.
m_impl->map = WTFMove(items);
- for (auto& pair : m_impl->map) {
- ASSERT(m_impl->currentSize + pair.key.sizeInBytes() + pair.value.sizeInBytes() >= m_impl->currentSize);
- m_impl->currentSize += (pair.key.sizeInBytes() + pair.value.sizeInBytes());
+ for (auto& [key, value] : m_impl->map) {
+ newSize += key.sizeInBytes();
+ newSize += value.sizeInBytes();
}
+ m_impl->currentSize = newSize;
return;
}
- for (auto& item : items) {
- auto& key = item.key;
- auto& value = item.value;
-
- ASSERT(m_impl->currentSize + key.sizeInBytes() + value.sizeInBytes() >= m_impl->currentSize);
- m_impl->currentSize += (key.sizeInBytes() + value.sizeInBytes());
-
+ for (auto& [key, value] : items) {
+ newSize += key.sizeInBytes();
+ newSize += value.sizeInBytes();
auto result = m_impl->map.add(WTFMove(key), WTFMove(value));
- ASSERT_UNUSED(result, result.isNewEntry); // True if the key didn't exist previously.
+ ASSERT_UNUSED(result, result.isNewEntry);
}
+ m_impl->currentSize = newSize;
}
Ref<StorageMap::Impl> StorageMap::Impl::copy() const
Modified: branches/safari-613.2.6.0-branch/Tools/ChangeLog (292821 => 292822)
--- branches/safari-613.2.6.0-branch/Tools/ChangeLog 2022-04-13 19:43:31 UTC (rev 292821)
+++ branches/safari-613.2.6.0-branch/Tools/ChangeLog 2022-04-13 19:53:16 UTC (rev 292822)
@@ -1,3 +1,55 @@
+2022-04-13 Kocsen Chung <kocsen_ch...@apple.com>
+
+ Cherry-pick r292721. rdar://problem/88249235
+
+ Fix size computation in WebCore::StorageMap
+ https://bugs.webkit.org/show_bug.cgi?id=239024
+ rdar://88249235
+
+ Reviewed by Chris Dumez.
+
+ Source/WebCore:
+
+ We use currentSize to track size for StorageMap. There are a few issues in current implementation that can make
+ currentSize incorrect and may lead to overflow:
+ 1. When computing size of key, StorageMap uses parameter key instead of stored key. The problem is that
+ two Strings can be evaluated to equal while their sizeInBytes() value is different, when one String is 8-bit and
+ the other is 16-bit. That means removeItem() may decrease currentSize by wrong number (e.g setItem() with an
+ 8-bit key, converting the key to 16-bit, removeItem() with the key). To fix this, StorageMap now always uses
+ stored key for computation.
+ 2. When map.take(key) or map.get(key) returns null string, StorageMap takes it as the key does not exist and
+ will not correctly update currentSize, but user of WebCore::StorageMap may store null string as value. To fix
+ this, StorageMap now check if key exists with find() function.
+ 3. StorageMap only uses CheckedUint32 in setItem(), but removeItem() and importItem() may cause overflow in
+ currentSize as mentioned above, and the error will not be caught until setItem() is called. To fix this,
+ StorageMap now uses CheckedUint32 in all places that update currentSize.
+
+ New test: WKWebView.LocalStorageNoSizeOverflow
+
+ * storage/StorageMap.cpp:
+ (WebCore::StorageMap::setItem):
+ (WebCore::StorageMap::removeItem):
+ (WebCore::StorageMap::importItems):
+
+ Tools:
+
+ * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
+ (TEST):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292721 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-04-11 Sihui Liu <sihui_...@apple.com>
+
+ Fix size computation in WebCore::StorageMap
+ https://bugs.webkit.org/show_bug.cgi?id=239024
+ rdar://88249235
+
+ Reviewed by Chris Dumez.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
+ (TEST):
+
2022-04-07 Alan Coon <alanc...@apple.com>
Cherry-pick r291886. rdar://problem/72814440
Modified: branches/safari-613.2.6.0-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm (292821 => 292822)
--- branches/safari-613.2.6.0-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm 2022-04-13 19:43:31 UTC (rev 292821)
+++ branches/safari-613.2.6.0-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm 2022-04-13 19:53:16 UTC (rev 292822)
@@ -454,6 +454,62 @@
runTest(false);
}
+TEST(WKWebView, LocalStorageNoSizeOverflow)
+{
+ readyToContinue = false;
+ [[WKWebsiteDataStore defaultDataStore] removeDataOfTypes:[WKWebsiteDataStore allWebsiteDataTypes] modifiedSince:[NSDate distantPast] completionHandler:^() {
+ readyToContinue = true;
+ }];
+ TestWebKitAPI::Util::run(&readyToContinue);
+
+ NSString *htmlString = @"<script> \
+ const key = '\u00FF\u00FF'; \
+ function onStorage() { window.webkit.messageHandlers.testHandler.postMessage('storage'); } \
+ function setItem() { localStorage.setItem(key, 'value'); } \
+ function removeItem() { localStorage.removeItem(localStorage.key(0)); } \
+ function getItem() { return localStorage.getItem(key) ? localStorage.getItem(key) : '[null]'; } \
+ window.addEventListener('storage', onStorage);\
+ window.webkit.messageHandlers.testHandler.postMessage(getItem()); \
+ </script>";
+ auto handler = adoptNS([[LocalStorageMessageHandler alloc] init]);
+ auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [[configuration userContentController] addScriptMessageHandler:handler.get() name:@"testHandler"];
+
+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+ [webView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"http://webkit.org"]];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+ EXPECT_WK_STREQ(@"[null]", [lastScriptMessage body]);
+ receivedScriptMessage = false;
+
+ [webView evaluateJavaScript:@"setItem()" completionHandler:^(NSNumber *result, NSError *error) {
+ receivedScriptMessage = true;
+ }];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+ receivedScriptMessage = false;
+
+ auto secondWebView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+ [secondWebView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"http://webkit.org"]];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+ EXPECT_WK_STREQ(@"value", [lastScriptMessage body]);
+ receivedScriptMessage = false;
+
+ [secondWebView evaluateJavaScript:@"removeItem()" completionHandler:nil];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+ EXPECT_WK_STREQ(@"storage", [lastScriptMessage body]);
+ receivedScriptMessage = false;
+
+ [secondWebView evaluateJavaScript:@"setItem()" completionHandler:nil];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+ EXPECT_WK_STREQ(@"storage", [lastScriptMessage body]);
+ receivedScriptMessage = false;
+
+ [webView evaluateJavaScript:@"getItem()" completionHandler:^(NSString* result, NSError *error) {
+ EXPECT_WK_STREQ("value", [result UTF8String]);
+ receivedScriptMessage = true;
+ }];
+ TestWebKitAPI::Util::run(&receivedScriptMessage);
+}
+
#if PLATFORM(IOS_FAMILY)
TEST(WKWebView, LocalStorageDirectoryExcludedFromBackup)
