0

I have a c++ node add on that uses the Nan library. I have a function that needs to return a buffer. The simplest version of it is as follows (code edited as per comments):

NAN_METHOD(Test) {
    char * retVal = (char*)malloc(100 * sizeof(char));
    info.GetReturnValue().Set(Nan::NewBuffer(retVal, 100 *sizeof(char)).ToLocalChecked());
}

where the union is just used as an easy way to reinterpret the bytes. According to the documentation, Nan::NewBuffer assumes ownership of the memory so there is no need to free the memory manually. However when I run my node code that uses this function, my memory skyrockets, even when I force the garbage collector to run via global.gc(); The node code to produce the error is extremely simple:

const addon = require("addon");
for (let i = 0; i < 100000000; i++) {
  if(i % (1000000) === 0){
    console.log(i);
    try {
      global.gc();
    } catch (e) {
      console.log("error garbage collecting");
      process.exit();
    }
  }
  const buf = addon.Test();
}

Any help would be appreciated.

cershif
  • 154
  • 1
  • 13
  • (1) the gc routine calls `free` not `delete` (_"...data will be disposed of via a call to free()..."_); (2) aliasing the pointer through a `union` is Undefined Behaviour; (3) even it the gc routine called `delete` it would be trying to delete the aliased pointer which is UB. – Richard Critten Jan 19 '21 at 19:02
  • @RichardCritten thank you. How would I achieve the desired result? would a reinterpret cast work? The point about delete vs. free is true, but does not hurt me here, as there is nothing to destruct. – cershif Jan 19 '21 at 19:10
  • If you don't pair `new/delete` and `malloc/free` then the code is in UB land and there is no point in trying to reason about it. – Richard Critten Jan 19 '21 at 19:18
  • @RichardCritten ok so the solution would be to use malloc instead of new. I will try that. – cershif Jan 19 '21 at 19:20
  • @RichardCritten same leak, after changing to malloc (edited in the question) – cershif Jan 19 '21 at 19:27

2 Answers2

1

After much experimentation and research, I discovered this postenter link description here which basically states that the promise to free the memory that is passed into Nan::NewBuffer is just a lie. Using Nan::CopyBuffer instead of Nan::NewBuffer solves the problem at the cost of a memcpy. So essentially, the answer is that Nan::NewBuffer is broken and you shouldn't use it. Use Nan::CopyBuffer instead.

cershif
  • 154
  • 1
  • 13
0

In IT, we tend to call a lie in the documentation a bug.

This question has been the subject of a Node.js issue - https://github.com/nodejs/node/issues/40936

The answer is that the deallocation of the underlying buffer happens in the C++ equivalent of a JS SetImmediate handler and if your code is completely synchronous - which is your case - the buffers won't be freed until the program end.

You correctly found that Nan::CopyBuffer does not suffer from this problem.

Alas, this cannot be easily fixed, as Node.js does not know how this buffer was allocated and cannot call its callback from the garbage collector context.

If you are using Nan::NewBuffer I also suggest to look at this issue: https://github.com/nodejs/node/issues/40926 in which another closely related pitfall is discussed.

mmomtchev
  • 2,497
  • 1
  • 8
  • 23