Title: [292721] trunk
- Revision
- 292721
- Author
- sihui_...@apple.com
- Date
- 2022-04-11 13:59:13 -0700 (Mon, 11 Apr 2022)
Log Message
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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (292720 => 292721)
--- trunk/Source/WebCore/ChangeLog 2022-04-11 20:49:06 UTC (rev 292720)
+++ trunk/Source/WebCore/ChangeLog 2022-04-11 20:59:13 UTC (rev 292721)
@@ -1,3 +1,32 @@
+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-11 Yusuke Suzuki <ysuz...@apple.com>
[JSC] Reduce use of unnecessary cryptographicallyRandom numbers
Modified: trunk/Source/WebCore/storage/StorageMap.cpp (292720 => 292721)
--- trunk/Source/WebCore/storage/StorageMap.cpp 2022-04-11 20:49:06 UTC (rev 292720)
+++ trunk/Source/WebCore/storage/StorageMap.cpp 2022-04-11 20:59:13 UTC (rev 292721)
@@ -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: trunk/Tools/ChangeLog (292720 => 292721)
--- trunk/Tools/ChangeLog 2022-04-11 20:49:06 UTC (rev 292720)
+++ trunk/Tools/ChangeLog 2022-04-11 20:59:13 UTC (rev 292721)
@@ -1,3 +1,14 @@
+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-11 Wenson Hsieh <wenson_hs...@apple.com>
[Mail Compose] Preserve attachment identifiers when cloning attachment-backed images
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm (292720 => 292721)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm 2022-04-11 20:49:06 UTC (rev 292720)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm 2022-04-11 20:59:13 UTC (rev 292721)
@@ -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)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
