Skip to content

Commit 7b01344

Browse files
pgriesspgriess
committed
Detect circular references and fail to pack.
- Tag v8::Object instances that we have seen during the current serialization run so that we can detect circular references. Store tags usign the hidden field facility in v8::Object.
1 parent 64cee77 commit 7b01344

2 files changed

Lines changed: 92 additions & 18 deletions

File tree

‎src/msgpack.cc‎

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,33 @@
99
using namespace v8;
1010
using namespace node;
1111

12+
static Persistent<String> msgpack_tag_symbol;
13+
14+
#define CHECK_CIRCULAR_REFS(o, t) \
15+
do { \
16+
Local<Value> ov = (o)->GetHiddenValue(msgpack_tag_symbol); \
17+
if (!ov.IsEmpty() && ov->Equals(t)) { \
18+
return false; \
19+
} \
20+
(o)->SetHiddenValue(msgpack_tag_symbol, (t)); \
21+
} while(0)
22+
1223
// Convert a V8 object to a MessagePack object.
1324
//
1425
// This method is recursive. It will probably blow out the stack on objects
1526
// with extremely deep nesting.
16-
static void
17-
v8_to_msgpack(Handle<Value> v8obj, msgpack_object *mo, msgpack_zone *mz) {
27+
//
28+
// This method can detect circular references in provided objects. It utilizes
29+
// the v8::Object::SetHiddenValue() facility to tag each encountered object
30+
// with a value unique to this serialization run. We leave the tags attached to
31+
// each object for ease-of-implementation (otherwise we'd have to track each
32+
// tagged v8::Object instance and un-tag at the end). This decision can be
33+
// revisited in the future if this proves problematic. I'd expect most objects
34+
// being packed to be short-lived anyway.
35+
//
36+
// Returns false if we detected a circular reference; true otherwise.
37+
static bool
38+
v8_to_msgpack(Handle<Value> v8obj, msgpack_object *mo, msgpack_zone *mz, Handle<Value> tag) {
1839
if (v8obj->IsUndefined() || v8obj->IsNull()) {
1940
mo->type = MSGPACK_OBJECT_NIL;
2041
} else if (v8obj->IsBoolean()) {
@@ -39,7 +60,10 @@ v8_to_msgpack(Handle<Value> v8obj, msgpack_object *mo, msgpack_zone *mz) {
3960

4061
DecodeWrite((char*) mo->via.raw.ptr, mo->via.raw.size, v8obj, UTF8);
4162
} else if (v8obj->IsArray()) {
42-
Local<Array> a = Local<Array>::Cast(v8obj->ToObject());
63+
Local<Object> o = v8obj->ToObject();
64+
Local<Array> a = Local<Array>::Cast(o);
65+
66+
CHECK_CIRCULAR_REFS(o, tag);
4367

4468
mo->type = MSGPACK_OBJECT_ARRAY;
4569
mo->via.array.size = a->Length();
@@ -50,12 +74,16 @@ v8_to_msgpack(Handle<Value> v8obj, msgpack_object *mo, msgpack_zone *mz) {
5074

5175
for (int i = 0; i < a->Length(); i++) {
5276
Local<Value> v = a->Get(i);
53-
v8_to_msgpack(v, &mo->via.array.ptr[i], mz);
77+
if (!v8_to_msgpack(v, &mo->via.array.ptr[i], mz, tag)) {
78+
return false;
79+
}
5480
}
5581
} else {
5682
Local<Object> o = v8obj->ToObject();
5783
Local<Array> a = o->GetPropertyNames();
5884

85+
CHECK_CIRCULAR_REFS(o, tag);
86+
5987
mo->type = MSGPACK_OBJECT_MAP;
6088
mo->via.map.size = a->Length();
6189
mo->via.map.ptr = (msgpack_object_kv*) msgpack_zone_malloc(
@@ -66,10 +94,17 @@ v8_to_msgpack(Handle<Value> v8obj, msgpack_object *mo, msgpack_zone *mz) {
6694
for (int i = 0; i < a->Length(); i++) {
6795
Local<Value> k = a->Get(i);
6896

69-
v8_to_msgpack(k, &mo->via.map.ptr[i].key, mz);
70-
v8_to_msgpack(o->Get(k), &mo->via.map.ptr[i].val, mz);
97+
if (!v8_to_msgpack(k, &mo->via.map.ptr[i].key, mz, tag)) {
98+
return false;
99+
}
100+
101+
if (!v8_to_msgpack(o->Get(k), &mo->via.map.ptr[i].val, mz, tag)) {
102+
return false;
103+
}
71104
}
72105
}
106+
107+
return true;
73108
}
74109

75110
// Convert a MessagePack object to a V8 object.
@@ -138,11 +173,14 @@ msgpack_to_v8(msgpack_object *mo) {
138173
// serialized to the same bytestream, back-ty-back.
139174
static Handle<Value>
140175
pack(const Arguments &args) {
176+
static int64_t tag_i = 0;
177+
141178
HandleScope scope;
142179

143180
msgpack_packer pk;
144181
msgpack_sbuffer sbuf;
145182
msgpack_zone mz;
183+
Local<Value> tag = Integer::New(++tag_i);
146184

147185
msgpack_sbuffer_init(&sbuf);
148186
msgpack_packer_init(&pk, &sbuf, msgpack_sbuffer_write);
@@ -152,7 +190,12 @@ pack(const Arguments &args) {
152190
for (int i = 0; i < args.Length(); i++) {
153191
msgpack_object mo;
154192

155-
v8_to_msgpack(args[0], &mo, &mz);
193+
if (!v8_to_msgpack(args[0], &mo, &mz, tag)) {
194+
return ThrowException(Exception::TypeError(String::New(
195+
"Cowardly refusing to pack object with circular reference"
196+
)));
197+
}
198+
156199
if (msgpack_pack_object(&pk, mo)) {
157200
return ThrowException(Exception::Error(
158201
String::New("Error serializaing object")));
@@ -211,6 +254,8 @@ init(Handle<Object> target) {
211254

212255
NODE_SET_METHOD(target, "pack", pack);
213256
NODE_SET_METHOD(target, "unpack", unpack);
257+
258+
msgpack_tag_symbol = NODE_PSYMBOL("msgpack::tag");
214259
}
215260

216261
// vim:ts=4 sw=4 et

‎test.js‎

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,48 @@
11
var assert = require('assert');
22
var msgpack = require('msgpack');
33

4-
var test = function(v) {
4+
var testEqual = function(v) {
55
var vv = msgpack.unpack(msgpack.pack(v));
66

77
assert.deepEqual(vv, v);
88
};
99

10-
test('abcdef');
11-
test(123);
12-
test(null);
13-
test(-1243.111);
14-
test(-123);
15-
test(true);
16-
test(false);
17-
test([1, 2, 3]);
18-
test([1, 'abc', false, null]);
19-
test({'a' : [1, 2, 3], 'b' : 'cdef', 'c' : {'nuts' : 'qqq'}});
10+
var testCircular = function(v) {
11+
try {
12+
msgpack.pack(v);
13+
assert.ok(false, 'expected exception');
14+
} catch (e) {
15+
assert.equal(
16+
e.message,
17+
'Cowardly refusing to pack object with circular reference'
18+
);
19+
}
20+
}
21+
22+
testEqual('abcdef');
23+
testEqual(123);
24+
testEqual(null);
25+
testEqual(-1243.111);
26+
testEqual(-123);
27+
testEqual(true);
28+
testEqual(false);
29+
testEqual([1, 2, 3]);
30+
testEqual([1, 'abc', false, null]);
31+
testEqual({'a' : [1, 2, 3], 'b' : 'cdef', 'c' : {'nuts' : 'qqq'}});
32+
33+
// Make sure we're catching circular references for arrays
34+
var a = [1, 2, 3, 4];
35+
a.push(a);
36+
testCircular(a);
37+
38+
// Make sure we're catching circular references in objects
39+
var d = {}
40+
d.qqq = d;
41+
testCircular(d);
42+
43+
// Make sure we can serialize the same object repeatedly and that our circular
44+
// reference marking algorithm doesn't get in the way
45+
var d = {};
46+
for (var i = 0; i < 10; i++) {
47+
msgpack.pack(d);
48+
}

0 commit comments

Comments
 (0)