Merge pull request #6977 from electron/ipc-cycle-detection

Improve cycle detection in IPC arguments

atom/common/native_mate_converters/v8_value_converter.cc

@@ -49,33 +49,83 @@ class V8ValueConverter::FromV8ValueState {
   // other handle B in the map points to the same object as A. Note that A can
   // be unique even if there already is another handle with the same identity
   // hash (key) in the map, because two objects can have the same hash.
-  bool UpdateAndCheckUniqueness(v8::Local<v8::Object> handle) {
+  bool AddToUniquenessCheck(v8::Local<v8::Object> handle) {
-    typedef HashToHandleMap::const_iterator Iterator;
+    int hash;
-    int hash = handle->GetIdentityHash();
+    auto iter = GetIteratorInMap(handle, &hash);
-    // We only compare using == with handles to objects with the same identity
+    if (iter != unique_map_.end())
-    // hash. Different hash obviously means different objects, but two objects
+      return false;
-    // in a couple of thousands could have the same identity hash.
+
-    std::pair<Iterator, Iterator> range = unique_map_.equal_range(hash);
-    for (auto it = range.first; it != range.second; ++it) {
-      // Operator == for handles actually compares the underlying objects.
-      if (it->second == handle)
-        return false;
-    }
     unique_map_.insert(std::make_pair(hash, handle));
     return true;
   }
 
+  bool RemoveFromUniquenessCheck(v8::Local<v8::Object> handle) {
+    int unused_hash;
+    auto iter = GetIteratorInMap(handle, &unused_hash);
+    if (iter == unique_map_.end())
+      return false;
+    unique_map_.erase(iter);
+    return true;
+  }
+
   bool HasReachedMaxRecursionDepth() {
     return max_recursion_depth_ < 0;
   }
 
  private:
-  typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
+  using HashToHandleMap = std::multimap<int, v8::Local<v8::Object>>;
+  using Iterator = HashToHandleMap::const_iterator;
+
+  Iterator GetIteratorInMap(v8::Local<v8::Object> handle, int* hash) {
+    *hash = handle->GetIdentityHash();
+    // We only compare using == with handles to objects with the same identity
+    // hash. Different hash obviously means different objects, but two objects
+    // in a couple of thousands could have the same identity hash.
+    std::pair<Iterator, Iterator> range = unique_map_.equal_range(*hash);
+    for (auto it = range.first; it != range.second; ++it) {
+      // Operator == for handles actually compares the underlying objects.
+      if (it->second == handle)
+        return it;
+    }
+    // Not found.
+    return unique_map_.end();
+  }
+
   HashToHandleMap unique_map_;
 
   int max_recursion_depth_;
 };
 
+// A class to ensure that objects/arrays that are being converted by
+// this V8ValueConverterImpl do not have cycles.
+//
+// An example of cycle: var v = {}; v = {key: v};
+// Not an example of cycle: var v = {}; a = [v, v]; or w = {a: v, b: v};
+class V8ValueConverter::ScopedUniquenessGuard {
+ public:
+  ScopedUniquenessGuard(V8ValueConverter::FromV8ValueState* state,
+                        v8::Local<v8::Object> value)
+      : state_(state),
+        value_(value),
+        is_valid_(state_->AddToUniquenessCheck(value_)) {}
+  ~ScopedUniquenessGuard() {
+    if (is_valid_) {
+      bool removed = state_->RemoveFromUniquenessCheck(value_);
+      DCHECK(removed);
+    }
+  }
+
+  bool is_valid() const { return is_valid_; }
+
+ private:
+  typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
+  V8ValueConverter::FromV8ValueState* state_;
+  v8::Local<v8::Object> value_;
+  bool is_valid_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedUniquenessGuard);
+};
+
 V8ValueConverter::V8ValueConverter()
     : reg_exp_allowed_(false),
       function_allowed_(false),
@@ -281,7 +331,8 @@ base::Value* V8ValueConverter::FromV8Array(
     v8::Local<v8::Array> val,
     FromV8ValueState* state,
     v8::Isolate* isolate) const {
-  if (!state->UpdateAndCheckUniqueness(val))
+  ScopedUniquenessGuard uniqueness_guard(state, val);
+  if (!uniqueness_guard.is_valid())
     return base::Value::CreateNullValue().release();
 
   std::unique_ptr<v8::Context::Scope> scope;
@@ -328,7 +379,8 @@ base::Value* V8ValueConverter::FromV8Object(
     v8::Local<v8::Object> val,
     FromV8ValueState* state,
     v8::Isolate* isolate) const {
-  if (!state->UpdateAndCheckUniqueness(val))
+  ScopedUniquenessGuard uniqueness_guard(state, val);
+  if (!uniqueness_guard.is_valid())
     return base::Value::CreateNullValue().release();
 
   std::unique_ptr<v8::Context::Scope> scope;

atom/common/native_mate_converters/v8_value_converter.h

@@ -32,6 +32,7 @@ class V8ValueConverter {
 
  private:
   class FromV8ValueState;
+  class ScopedUniquenessGuard;
 
   v8::Local<v8::Value> ToV8ValueImpl(v8::Isolate* isolate,
                                      const base::Value* value) const;

spec/api-ipc-spec.js

@@ -325,6 +325,22 @@ describe('ipc module', function () {
       })
       ipcRenderer.send('message', document.location)
     })
+
+    it('can send objects that both reference the same object', function (done) {
+      const child = {hello: 'world'}
+      const foo = {name: 'foo', child: child}
+      const bar = {name: 'bar', child: child}
+      const array = [foo, bar]
+
+      ipcRenderer.once('message', function (event, arrayValue, fooValue, barValue, childValue) {
+        assert.deepEqual(arrayValue, array)
+        assert.deepEqual(fooValue, foo)
+        assert.deepEqual(barValue, bar)
+        assert.deepEqual(childValue, child)
+        done()
+      })
+      ipcRenderer.send('message', array, foo, bar, child)
+    })
   })
 
   describe('ipc.sendSync', function () {

spec/static/main.js

@@ -35,8 +35,8 @@ process.stdout
 // Access console to reproduce #3482.
 console
 
-ipcMain.on('message', function (event, arg) {
+ipcMain.on('message', function (event, ...args) {
-  event.sender.send('message', arg)
+  event.sender.send('message', ...args)
 })
 
 // Write output to file if OUTPUT_TO_FILE is defined.