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

diff --git a/json_patch.c b/json_patch.c
index 32771bef6250fa55a944b58fb06974f675e1f7f5..7eec770ccea64354564d6d31b2d4f55b8720c786 100644 (file)
--- a/json_patch.c
+++ b/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;

diff --git a/tests/json_patch_tests.json b/tests/json_patch_tests.json
index 8cafb93c961ec6028f173a15e7919185203779e3..a554ab96f987fff78c4b55c0d718bc73b703adb1 100644 (file)
--- a/tests/json_patch_tests.json
+++ b/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": {}}}
     }
 
 ]

diff --git a/tests/test_json_patch.expected b/tests/test_json_patch.expected
index 523d276b25def73e624a88cdef00bb31b9110cd6..69777eb3c70dd662407fc14eb7d77d1cad7e3421 100644 (file)
--- a/tests/test_json_patch.expected
+++ b/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