Cherry-pick r293736. rdar://80891555

StorageMap::removeItem may fail to remove item from map https://bugs.webkit.org/show_bug.cgi?id=239982 rdar://80891555 Reviewed by Chris Dumez. Source/WebCore: We may have updated m_impl, but we don't update iterator for removal. In this case, item is not removed from map, but currentSize is updated. The mismatch between currentSize and actual size of the map may lead to underflow and overflow in currentSize when item is added or removed later. Test: storage/domstorage/sessionstorage/window-open-remove-item.html * storage/StorageMap.cpp: (WebCore::StorageMap::removeItem): LayoutTests: * storage/domstorage/sessionstorage/resources/window-open-remove-item.html: Added. * storage/domstorage/sessionstorage/window-open-remove-item-expected.txt: Added. * storage/domstorage/sessionstorage/window-open-remove-item.html: Added. Canonical link: https://commits.webkit.org/250224@main git-svn-id: https://svn.webkit.org/repository/webkit/branches/safari-7613.3.1.0-branch@294041 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 11, 2022 · 42f2ede · 42f2ede
1 parent 369b39e
commit 42f2ede
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 1 deletion.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2022-05-03  Sihui Liu  <sihui_liu@apple.com>
+
+        StorageMap::removeItem may fail to remove item from map
+        https://bugs.webkit.org/show_bug.cgi?id=239982
+        rdar://80891555
+
+        Reviewed by Chris Dumez.
+
+        * storage/domstorage/sessionstorage/resources/window-open-remove-item.html: Added.
+        * storage/domstorage/sessionstorage/window-open-remove-item-expected.txt: Added.
+        * storage/domstorage/sessionstorage/window-open-remove-item.html: Added.
+
 2022-04-26  Russell Epstein  <repstein@apple.com>
 
         Cherry-pick r290630. rdar://problem/89442583

diff --git a/LayoutTests/storage/domstorage/sessionstorage/resources/window-open-remove-item.html b/LayoutTests/storage/domstorage/sessionstorage/resources/window-open-remove-item.html
@@ -0,0 +1,15 @@
+<html>
+<body>
+<script>
+
+if (sessionStorage.getItem("key") != "value")
+    localStorage.setItem("result", "fail");
+else {
+    sessionStorage.removeItem("key");
+    sessionStorage.setItem("key", "newValue");
+    localStorage.setItem("result", "pass");
+}
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/storage/domstorage/sessionstorage/window-open-remove-item-expected.txt b/LayoutTests/storage/domstorage/sessionstorage/window-open-remove-item-expected.txt
@@ -0,0 +1,11 @@
+Test verifies that process does not crash when item is updated in another window
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS localStorage.getItem('result') is "pass"
+PASS sessionStorage.getItem('key') is "value"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/storage/domstorage/sessionstorage/window-open-remove-item.html b/LayoutTests/storage/domstorage/sessionstorage/window-open-remove-item.html
@@ -0,0 +1,24 @@
+<html>
+<head>
+<script src="../../../resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description("Test verifies that process does not crash when item is updated in another window");
+jsTestIsAsync = true;
+
+sessionStorage.clear();
+localStorage.clear();
+
+sessionStorage.setItem("key", "value");
+window.open("resources/window-open-remove-item.html");
+
+addEventListener('storage', event => { 
+    shouldBeEqualToString("localStorage.getItem('result')", "pass");
+    shouldBeEqualToString("sessionStorage.getItem('key')", "value");
+    finishJSTest();
+});
+
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,20 @@
+2022-05-03  Sihui Liu  <sihui_liu@apple.com>
+
+        StorageMap::removeItem may fail to remove item from map
+        https://bugs.webkit.org/show_bug.cgi?id=239982
+        rdar://80891555
+
+        Reviewed by Chris Dumez.
+
+        We may have updated m_impl, but we don't update iterator for removal. In this case, item is not removed from
+        map, but currentSize is updated. The mismatch between currentSize and actual size of the map may lead to
+        underflow and overflow in currentSize when item is added or removed later.
+
+        Test: storage/domstorage/sessionstorage/window-open-remove-item.html
+
+        * storage/StorageMap.cpp:
+        (WebCore::StorageMap::removeItem):
+
 2022-05-04  Alan Coon  <alancoon@apple.com>
 
         Apply patch. rdar://problem/75450208

diff --git a/Source/WebCore/storage/StorageMap.cpp b/Source/WebCore/storage/StorageMap.cpp
@@ -138,7 +138,7 @@ void StorageMap::removeItem(const String& key, String& oldValue)
     if (m_impl->refCount() > 1)
         m_impl = m_impl->copy();
 
-    m_impl->map.remove(iter);
+    m_impl->map.remove(key);
     m_impl->currentSize = newSize;
     invalidateIterator();
 }