fix: possible SIGILL and memory leak#204
Conversation
bnoordhuis
left a comment
There was a problem hiding this comment.
Left some comments but mostly LGTM.
Thanks for doing this and sorry for the delay, I'm afraid the notification got buried.
| class FSEvents : public Nan::ObjectWrap { | ||
| public: | ||
| FSEvents(const char *path, Nan::Callback *handler); | ||
| FSEvents(const char *path); |
There was a problem hiding this comment.
Can you make this explicit?
|
|
||
| // locking.cc | ||
| bool lockStarted; | ||
| bool lockStarted = false; |
There was a problem hiding this comment.
Can you do this in the constructor? This is a C++11 construct and we (nominally) still support Node.js v0.10 and v0.12, which are C++03.
| (void) object->Set(object->CreationContext(), key, value); | ||
| #else | ||
| object->Set(key, value); | ||
| #endif |
There was a problem hiding this comment.
There are Nan methods for this (Nan::Get() and Nan::Set().)
| if (!handler) return; | ||
| Nan::HandleScope handle_scope; | ||
| v8::Local<v8::Object> object = handle(); | ||
| Nan::Callback handler(Get(object, Nan::New<v8::String>("handler").ToLocalChecked()).As<v8::Function>()); |
There was a problem hiding this comment.
Can you use Nan::To<v8::Function>(...)? It does an IsFunction() check before casting.
| FSEvents *fse = new FSEvents(**path, callback); | ||
| FSEvents *fse = new FSEvents(*path); | ||
| fse->Wrap(info.This()); | ||
| Set(info.This(), Nan::New<v8::String>("handler").ToLocalChecked(), info[1].As<v8::Function>()); |
There was a problem hiding this comment.
Not necessary to cast to v8::Function.
|
Thank you for you feedback. The C++11 initialization must have slipped through. I hope my changes satisfy your review. |
This PR fixes two bugs which I found while "stress-testing" the library with
and
The first one is a SIGILL signal generated in
pthread_mutex_destroybecause the mutex is not always initialized. This is caused by not initializinglockStartedwhich causes it to sometimes be true, depending on where the object was allocated. A simple fix is to initialize it tofalse.The other bug is a cyclic dependency which is hidden from v8 and therefore never freed. If the closure handed to FSEvents has a reference to the event and FSEvents saves the callback internally, a cyclic dependency is created. The problem arises, because v8 can only directly see the
closure->FSEventsdependency not the internal one. My fix is to "promote" the closure reference to the associated v8::Object, therefore allowing v8 to see the dependency. Before fixing this bug I had no prior experience with v8 so my solution might not be ideal.In general I did not see any contribution guidelines so I would be happy to amend these changes where necessary.