Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: possible SIGILL and memory leak #204

Merged
merged 3 commits into from Apr 26, 2018
Merged

fix: possible SIGILL and memory leak #204

merged 3 commits into from Apr 26, 2018

Conversation

Henje
Copy link
Contributor

@Henje Henje commented Mar 18, 2018

This PR fixes two bugs which I found while "stress-testing" the library with

var fsevents = require("fsevents");
while(true) {
  new fsevents.FSEvents("/", () => {});
}

and

var fsevents = require("fsevents");
while(true) {
  fsevents("/");
}

The first one is a SIGILL signal generated in pthread_mutex_destroy because the mutex is not always initialized. This is caused by not initializing lockStarted which causes it to sometimes be true, depending on where the object was allocated. A simple fix is to initialize it to false.

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->FSEvents dependency 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.

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments but mostly LGTM.

Thanks for doing this and sorry for the delay, I'm afraid the notification got buried.

fsevents.cc Outdated
public:
FSEvents(const char *path, Nan::Callback *handler);
FSEvents(const char *path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this explicit?

fsevents.cc Outdated
~FSEvents();

// locking.cc
bool lockStarted;
bool lockStarted = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/methods.cc Outdated
(void) object->Set(object->CreationContext(), key, value);
#else
object->Set(key, value);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are Nan methods for this (Nan::Get() and Nan::Set().)

src/methods.cc Outdated
Nan::HandleScope handle_scope;
v8::Local<v8::Object> object = handle();
Nan::Callback handler(Get(object, Nan::New<v8::String>("handler").ToLocalChecked()).As<v8::Function>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use Nan::To<v8::Function>(...)? It does an IsFunction() check before casting.

src/methods.cc Outdated
fse->Wrap(info.This());
Set(info.This(), Nan::New<v8::String>("handler").ToLocalChecked(), info[1].As<v8::Function>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary to cast to v8::Function.

@Henje
Copy link
Contributor Author

Henje commented Apr 15, 2018

Thank you for you feedback. The C++11 initialization must have slipped through. I hope my changes satisfy your review.

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

@es128 es128 merged commit c02045d into fsevents:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants