Issue 652 cycle detection doesnt catch cycles through collections

src/main/java/org/json/JSONArray.java

@@ -15,6 +15,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 
 /**
@@ -149,11 +150,27 @@ public JSONArray(String source) throws JSONException {
      *            A Collection.
      */
     public JSONArray(Collection<?> collection) {
+        this(collection, null);
+    }
+
+    /**
+     * Construct a JSONArray from a Collection, with an objectsRecord
+     * set to identify recursively defined objects.
+     * 
+     * It's package-private because it is called exclusively by
+     * {@code JSONObject#wrap(Object, Set)}.
+     *
+     * @param collection
+     *            A Collection.
+     * @param objectsRecord
+     *            The set of already recorded objects
+     */
+    JSONArray(Collection<?> collection, Set<Object> objectsRecord) {
         if (collection == null) {
             this.myArrayList = new ArrayList<Object>();
         } else {
             this.myArrayList = new ArrayList<Object>(collection.size());
-            this.addAll(collection, true);
+            this.addAll(collection, true, objectsRecord);
         }
     }
 
@@ -1577,7 +1594,8 @@ public List<Object> toList() {
     public boolean isEmpty() {
         return this.myArrayList.isEmpty();
     }
-
+
+
     /**
      * Add a collection's elements to the JSONArray.
      *
@@ -1589,10 +1607,26 @@ public boolean isEmpty() {
      *            
      */
     private void addAll(Collection<?> collection, boolean wrap) {
+        this.addAll(collection, wrap, null);
+    }
+
+    /**
+     * In addition to adding a collection to the JSONArray in the manner of {@code #addAll(Collection, boolean)},
+     * propagates the objectsRecord to {@code JSONObject#wrap(Object, Set)} to identify recursively defined objects.
+     * 
+     * @param collection
+     *            A Collection.
+     * @param wrap
+     *            {@code true} to call {@link JSONObject#wrap(Object)} for each item,
+     *            {@code false} to add the items directly
+     * @param objectsRecord
+     *            The set of already recorded objects
+     */
+    private void addAll(Collection<?> collection, boolean wrap, Set<Object> objectsRecord) {
         this.myArrayList.ensureCapacity(this.myArrayList.size() + collection.size());
         if (wrap) {
             for (Object o: collection){
-                this.put(JSONObject.wrap(o));
+                this.put(JSONObject.wrap(o, objectsRecord));
             }
         } else {
             for (Object o: collection){

src/main/java/org/json/JSONObject.java

@@ -1545,6 +1545,20 @@ && isValidMethodName(method.getName())) {
                             // itself DFS recursive
                             if (objectsRecord.contains(result)) {
                                 throw recursivelyDefinedObjectException(key);
+                            } else if (result instanceof Collection) {
+                                Collection<?> collection = (Collection<?>) result;
+                                // We're modifying the collection to check it,
+                                // so we want to not affect the original collection.
+                                Set<Object> set =  Collections.newSetFromMap(new IdentityHashMap<Object, Boolean>());
+                                set.addAll(collection);
+
+                                for (Object object : objectsRecord) {
+                                    if (object instanceof Collection 
+                                            && set.removeAll((Collection<?>) object)
+                                            || set.remove(object)) {
+                                        throw recursivelyDefinedObjectException(key);
+                                    }
+                                }
                             }
 
                             objectsRecord.add(result);
@@ -2452,7 +2466,20 @@ public static Object wrap(Object object) {
         return wrap(object, null);
     }
 
-    private static Object wrap(Object object, Set<Object> objectsRecord) {
+    /**
+     * Wraps the object much like {@code #wrap(Object)}, but propagates an objectsRecord set
+     * to identify recursively defined objects.
+     * 
+     * It's package private because other classes in this package may need to further propagate
+     * the set.
+     * 
+     * @param object
+     *            The object to wrap
+     * @param objectsRecord
+     *            The set of already recorded objects
+     * @return The wrapped value
+     */
+    static Object wrap(Object object, Set<Object> objectsRecord) {
         try {
             if (NULL.equals(object)) {
                 return NULL;
@@ -2470,7 +2497,7 @@ private static Object wrap(Object object, Set<Object> objectsRecord) {
 
             if (object instanceof Collection) {
                 Collection<?> coll = (Collection<?>) object;
-                return new JSONArray(coll);
+                return new JSONArray(coll, objectsRecord);
             }
             if (object.getClass().isArray()) {
                 return new JSONArray(object);

src/test/java/org/json/junit/JSONObjectRecursiveExceptionTest.java

@@ -0,0 +1,75 @@
+package org.json.junit;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.hamcrest.core.StringContains;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+public class JSONObjectRecursiveExceptionTest {
+    public static class RecursiveList {
+        public List<RecursiveList> getRecursiveList() {
+            return Collections.singletonList(this);
+        }
+    }
+
+    public static class RecursiveArray {
+        public RecursiveArray[] getRecursiveArray() {
+            return new RecursiveArray[] {this};
+        }
+    }
+
+    public static class RecursiveMap {
+        public Map<String, RecursiveMap> getRecursiveMap() {
+            Map<String, RecursiveMap> map = new HashMap<String, RecursiveMap>();
+            map.put("test", this);
+            return map;
+        }
+    }
+
+    public static class Recursive {
+        public Recursive getSelf() {
+            return this;
+        }
+    }
+
+    @Rule
+    public ExpectedException exceptionRule = ExpectedException.none();
+
+    @Before
+    public void before() {
+        exceptionRule.expect(JSONException.class);
+        exceptionRule.expectMessage(StringContains.containsString("JavaBean object contains recursively defined member variable of key"));
+    }
+
+
+    @Test
+    public void testRecursiveList() {          
+        new JSONObject(new RecursiveList());
+    }
+
+    @Ignore
+    @Test
+    public void testRecursiveArray() {        
+        new JSONObject(new RecursiveArray());
+    }
+
+    @Ignore
+    @Test
+    public void testRecursiveMap() {        
+        new JSONObject(new RecursiveMap());
+    }
+
+    @Test
+    public void testRecursive() {
+        new JSONObject(new Recursive());
+    }
+}