Repository navigation
Possible native memory leak of the msgpack_sbuffer on the throw paths of pack #25686
Description
Possible native memory leak of the msgpack_sbuffer on the throw paths of pack
I found a possible alloc-before-validate leak in pack. The function obtains an
msgpack_sbuffer *sb (either popped from the reuse pool or freshly allocated with
msgpack_sbuffer_new()) and only transfers ownership of it at the very end, when it wraps
sb->data in a Node Buffer with the _free_sbuf finalizer. If any argument fails to serialize
— which is user-triggerable, e.g. an object containing a circular reference — the function
returns via Nan::ThrowError(...) before reaching that point, so sb (and any bytes already
accumulated in sb->data) is neither freed nor returned to the pool. Every failed
msgpack.pack(...) call leaks one msgpack_sbuffer.
File: src/msgpack.cc
Function: pack (NAN_METHOD(pack))
static NAN_METHOD(pack) {
Nan::HandleScope scope;
msgpack_packer pk;
MsgpackZone mz;
msgpack_sbuffer *sb;
if (!sbuffers.empty()) {
sb = sbuffers.top();
sbuffers.pop();
} else {
sb = msgpack_sbuffer_new(); // <-- per-call heap allocation
}
msgpack_packer_init(&pk, sb, msgpack_sbuffer_write);
for (int i = 0; i < info.Length(); i++) {
msgpack_object mo;
try {
v8_to_msgpack(info[i], &mo, &mz._mz, 0);
} catch (MsgpackException e) {
return Nan::ThrowError(e.getThrownException()); // <-- sb leaked
}
if (msgpack_pack_object(&pk, mo)) {
return Nan::ThrowError("Error serializaing object"); // <-- sb leaked
}
}
Local<Object> slowBuffer = Nan::NewBuffer(
sb->data, sb->size, _free_sbuf, (void *)sb // ownership handed off only here
).ToLocalChecked();
return info.GetReturnValue().Set(slowBuffer);
}Path, line by line:
sbis acquired per call: reused from thesbufferspool (in which case the pool relinquishes
that entry) or newly heap-allocated bymsgpack_sbuffer_new().- The only cleanup route for
sbis the success path, whereNan::NewBuffer(sb->data, sb->size, _free_sbuf, sb)transfers ownership; when the returned Buffer is GC'd,_free_sbufeither
freessbor recycles it into the pool. MsgpackZone mzis RAII, but it only owns themsgpack_zone(mz._mz) — it does not own
sb.sbis a raw pointer with no RAII wrapper.- If
v8_to_msgpackthrowsMsgpackException(e.g.512 < ++depthfor a circular reference),
thecatchreturnsNan::ThrowError(...)andsbis dropped without being freed or pushed
back to the pool — a leak of themsgpack_sbufferstruct plus whateversb->datahad already
grown to from earlier successful arguments. - The
msgpack_pack_objecterror path leaks identically.
This is a synchronous method (no async worker / after-work handoff), the leak is per operation,
and it is reachable with ordinary input.
JS trigger:
const msgpack = require('msgpack');
const o = {};
o.self = o; // circular reference -> v8_to_msgpack throws at depth > 512
for (let i = 0; i < 1e6; i++) {
try { msgpack.pack(o); } catch (e) {} // each call leaks one msgpack_sbuffer
}Suggested fix: adopt sb in a RAII guard (or free/recycle it in the error paths). E.g. before
each return Nan::ThrowError(...), call _free_sbuf(sb->data, sb) (or reset and push sb back
onto sbuffers) so the buffer is reclaimed on every exit path, not only on success.
Activity
fix: restore contributors and cite msgpack-node#25686
feat: 2.0.0 security modernization (#25687)