Check that cb is a function, not non-empty

[sam-github]

This triggers a bug on node 6.x, see haraka/Haraka#1546 and #16, and this seems to work on older nodes as well as 6.

@bnoordhuis, does this look correct to you? I'm somewhat confused by nodejs/nan#104.

[sam-github]

diff --git a/core.cc b/core.cc
index e4904e3..4786530 100644
--- a/core.cc
+++ b/core.cc
@@ -30,7 +30,7 @@ class Worker: public Nan::AsyncWorker {
         void HandleOKCallback() {
             Nan::HandleScope scope;

-            if(callback->GetFunction()->IsFunction())
+            if(callback)
                 callback->Call(0, NULL);
         };

@@ -80,7 +80,10 @@ NAN_METHOD(SysLog) {

     int priority = info[0]->Int32Value();
     char* message = NULL;
-    Nan::Callback *callback = new Nan::Callback(info[2].As<Function>());
+    Nan::Callback *callback = NULL;
+
+    if (info[2]->IsFunction())
+        callback = new Nan::Callback(info[2].As<Function>());

     if(node::Buffer::HasInstance(info[1])) {
         message = dupBuf(info[1]);
@@ -90,7 +93,7 @@ NAN_METHOD(SysLog) {

     if (message) {
         Nan::AsyncQueueWorker(new Worker(callback, priority, message));
-    } else if(callback->GetFunction()->IsFunction()) {
+    } else if(callback) {
         callback->Call(0, NULL);
     }

also seems to work.

[bnoordhuis]

You can just omit HandleOKCallback, callback->Call(0, NULL) is the default implementation.

[sam-github]

the default implementation doesn't check to see if callback is NULL, so it segfaults if there is no callback

[bnoordhuis]

Yes, but that is why you should check the result of IsFunction() before creating the Nan::Callback instance. Blindly casting the function argument is sliding into UB territory.

[sam-github]

Not following. Check my repush. callback argument is mandatory to Worker, and the default implementation of HandleOKCallback just blindly uses it.

When I check IsFunction(), I have no callback, so don't want to cast the non-existent argument to a function, or in any way create a callback, but I have to pass something in as cb argument to the constructor.

[sam-github]

if info[2]->IsFunction() is false, then there is no callback... but I have to pass something into the worker ctor, and then its default HandleOKCallback() will try to use that something. So, I think I need my own implementation... unless its possible to create a no-op Nan::Callback()? Maybe else callback = new Nan::Callback()? ... but I just tried that, it compiles, but segvs.

[sam-github]

/to @bnoordhuis ... though I think you get notifications anyhow.

[bnoordhuis]

Ah, I see now. LGTM but you can replace the info[2]->IsFunction() check below with a callback != NULL.

[sam-github]

right, that's a relic of my new Nan::Callback() try, sorry.

[sam-github]

Thanks for the comments, Ben, and nice to hear from you, even if only at this distance. Maybe I'll see you in Europe some day, hope its going well for you out there in the wilds of northern Holland :-)

[bnoordhuis]

Thanks, Sam. You should come visit when you're in Europe, life on a farm is great!