CompactVariableMap::Handle's copy operator= leaks the previous data
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2019 22:41:17 +0000 (22:41 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2019 22:41:17 +0000 (22:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195398

Reviewed by Yusuke Suzuki.

The copy constructor was just assigning |this| to the new value,
forgetting to decrement the ref count of the thing pointed to by
the |this| handle. Based on Yusuke's suggestion, this patch refactors
the move constructor, move operator=, and copy operator= to use the
swap() primitive and the copy constructor primitive.

* parser/VariableEnvironment.cpp:
(JSC::CompactVariableMap::Handle::Handle):
(JSC::CompactVariableMap::Handle::swap):
(JSC::CompactVariableMap::Handle::operator=): Deleted.
* parser/VariableEnvironment.h:
(JSC::CompactVariableMap::Handle::Handle):
(JSC::CompactVariableMap::Handle::operator=):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/parser/VariableEnvironment.cpp
Source/JavaScriptCore/parser/VariableEnvironment.h

index 9c65026..ee6f354 100644 (file)
@@ -1,3 +1,24 @@
+2019-03-07  Saam Barati  <sbarati@apple.com>
+
+        CompactVariableMap::Handle's copy operator= leaks the previous data
+        https://bugs.webkit.org/show_bug.cgi?id=195398
+
+        Reviewed by Yusuke Suzuki.
+
+        The copy constructor was just assigning |this| to the new value,
+        forgetting to decrement the ref count of the thing pointed to by
+        the |this| handle. Based on Yusuke's suggestion, this patch refactors
+        the move constructor, move operator=, and copy operator= to use the
+        swap() primitive and the copy constructor primitive.
+
+        * parser/VariableEnvironment.cpp:
+        (JSC::CompactVariableMap::Handle::Handle):
+        (JSC::CompactVariableMap::Handle::swap):
+        (JSC::CompactVariableMap::Handle::operator=): Deleted.
+        * parser/VariableEnvironment.h:
+        (JSC::CompactVariableMap::Handle::Handle):
+        (JSC::CompactVariableMap::Handle::operator=):
+
 2019-03-07  Tadeu Zagallo  <tzagallo@apple.com>
 
         Lazily decode cached bytecode
index faddd14..8e69151 100644 (file)
@@ -193,26 +193,20 @@ CompactVariableMap::Handle::~Handle()
 }
 
 CompactVariableMap::Handle::Handle(const CompactVariableMap::Handle& other)
+    : m_environment(other.m_environment)
+    , m_map(other.m_map)
 {
-    *this = other;
-}
-
-CompactVariableMap::Handle& CompactVariableMap::Handle::operator=(const Handle& other)
-{
-    m_map = other.m_map;
-    m_environment = other.m_environment;
-
-    if (!m_map) {
-        ASSERT(!m_environment);
-        // This happens if `other` were moved into a different handle.
-        return *this;
+    if (m_map) {
+        auto iter = m_map->m_map.find(CompactVariableMapKey { *m_environment });
+        RELEASE_ASSERT(iter != m_map->m_map.end());
+        ++iter->value;
     }
+}
 
-    auto iter = m_map->m_map.find(CompactVariableMapKey { *m_environment });
-    RELEASE_ASSERT(iter != m_map->m_map.end());
-    ++iter->value;
-
-    return *this;
+CompactVariableMap::Handle::Handle(CompactVariableEnvironment& environment, CompactVariableMap& map)
+    : m_environment(&environment)
+    , m_map(&map)
+{ 
 }
 
 } // namespace JSC
index 9f90450..efaa5a7 100644 (file)
@@ -214,21 +214,26 @@ public:
     public:
         Handle() = default;
 
-        Handle(CompactVariableEnvironment& environment, CompactVariableMap& map)
-            : m_environment(&environment)
-            , m_map(&map)
-        { }
+        Handle(CompactVariableEnvironment&, CompactVariableMap&);
+
         Handle(Handle&& other)
-            : m_environment(other.m_environment)
-            , m_map(WTFMove(other.m_map))
         {
-            RELEASE_ASSERT(!!m_environment == !!m_map);
-            ASSERT(!other.m_map);
-            other.m_environment = nullptr;
+            swap(other);
+        }
+        Handle& operator=(Handle&& other)
+        {
+            Handle handle(WTFMove(other));
+            swap(handle);
+            return *this;
         }
 
         Handle(const Handle&);
-        Handle& operator=(const Handle&);
+        Handle& operator=(const Handle& other)
+        {
+            Handle handle(other);
+            swap(handle);
+            return *this;
+        }
 
         ~Handle();
 
@@ -240,6 +245,12 @@ public:
         }
 
     private:
+        void swap(Handle& other)
+        {
+            std::swap(other.m_environment, m_environment);
+            std::swap(other.m_map, m_map);
+        }
+
         CompactVariableEnvironment* m_environment { nullptr };
         RefPtr<CompactVariableMap> m_map;
     };