Merge pull request #931 from dxbjavid/patch-copy-deep-copy

deep copy values in json_patch copy op to avoid aliasing and cycles

json_patch.c

@@ -198,6 +198,7 @@ static int json_patch_apply_move_copy(struct json_object **res,
 	json_pointer_set_cb array_set_cb;
 	struct json_pointer_get_result from;
 	struct json_object *jfrom;
+	struct json_object *value;
 	const char *from_s;
 	size_t from_s_len;
 	int rc;
@@ -233,24 +234,39 @@ static int json_patch_apply_move_copy(struct json_object **res,
 
 	// Note: it's impossible for json_pointer to find the root obj, due
 	// to the path check above, so from.parent is guaranteed non-NULL
-	json_object_get(from.obj);
 
 	if (!move) {
+		/* RFC 6902 section 4.5: "copy" duplicates the value, it does
+		 * not share it.  Sharing the source via a reference (as a plain
+		 * json_object_get() would) lets a later op insert the value
+		 * beneath itself through a different pointer path, which the
+		 * from/path string check above can't detect because both paths
+		 * resolve to the same object, producing a reference cycle that
+		 * loops forever on serialisation and breaks teardown.  An
+		 * independent deep copy can never alias an existing node. */
+		value = NULL;
+		if (from.obj != NULL &&
+		    json_object_deep_copy(from.obj, &value, NULL) < 0) {
+			_set_err(ENOMEM, "Unable to copy value referenced by 'from' field");
+			return -1;
+		}
 		array_set_cb = json_object_array_insert_idx_cb;
 	} else {
+		json_object_get(from.obj);
+		value = from.obj;
 		rc = __json_patch_apply_remove(&from);
 		if (rc < 0) {
-			json_object_put(from.obj);
+			json_object_put(value);
 			return rc;
 		}
 		array_set_cb = json_object_array_move_cb;
 	}
 
-	rc = json_pointer_set_with_cb(res, path, from.obj, array_set_cb, 0, &from);
+	rc = json_pointer_set_with_cb(res, path, value, array_set_cb, 0, &from);
 	if (rc)
 	{
 		_set_err(errno, "Failed to set value at path referenced by 'path' field");
-		json_object_put(from.obj);
+		json_object_put(value);
 	}
 
 	return rc;

tests/json_patch_tests.json

@@ -535,6 +535,13 @@
        "doc": {"foo":"bar"},
        "patch": [{"op": "add", "path": "/FOO", "value": "BAR"}],
        "expected": {"foo": "bar", "FOO": "BAR"}
+    },
+
+    { "comment": "copy must duplicate the value, not alias it, so no cycle can form",
+       "doc": {"a": {"p": {}}},
+       "patch": [{"op": "copy", "from": "/a", "path": "/b"},
+                 {"op": "copy", "from": "/b", "path": "/a/p/x"}],
+       "expected": {"a": {"p": {"x": {"p": {}}}}, "b": {"p": {}}}
     }
 
 ]

tests/test_json_patch.expected

@@ -156,3 +156,4 @@ Testing 'Removing deep nonexistent path', doc '{ "foo": "bar" }' patch '[ { "op"
 Testing 'Removing nonexistent index', doc '[ "foo", "bar" ]' patch '[ { "op": "remove", "path": "\/2" } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Did not find element referenced by path field
 Testing 'Patch with different capitalisation than doc', doc '{ "foo": "bar" }' patch '[ { "op": "add", "path": "\/FOO", "value": "BAR" } ]' : OK
+Testing 'copy must duplicate the value, not alias it, so no cycle can form', doc '{ "a": { "p": { } } }' patch '[ { "op": "copy", "from": "\/a", "path": "\/b" }, { "op": "copy", "from": "\/b", "path": "\/a\/p\/x" } ]' : OK