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 thread-safety issues in native bindings replacing Nan::Persistent with v8::Eternal #1419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmnilin
Copy link

@rmnilin rmnilin commented Sep 7, 2024

This PR addresses thread-safety issues in the native bindings of the library. It replaces Nan::Persistent with v8::Eternal for constructor storage in cipher and decipher classes.

The previous implementation using Nan::Persistent was not thread-safe, leading to crashes and undefined behavior when these classes were accessed from multiple threads simultaneously. This was particularly problematic when ssh2 was used in multi-threaded environments, such as when imported in worker threads.

Key changes:

  1. Modified the constructor method to return a v8::Eternal instead of a Nan::Persistent.
  2. Updated the init function to use the Set method of the Eternal handle.

These changes ensure thread-safe storage of constructor handles without altering the intended functionality of the modules.

This PR addresses issue #1393 and resolves the reported crashes and segmentation faults related to multi-threaded use of the ssh2 library.

@mscdex
Copy link
Owner

mscdex commented Sep 7, 2024

I don't understand how using a v8::Eternal fixes a thread safety issue. AFAIK the only real difference between v8::Eternal and v8::Persistent is the lifetime of the value. v8::Eternal will always last the lifetime of the isolate whereas v8::Persistent will not necessarily.

@mscdex
Copy link
Owner

mscdex commented Sep 7, 2024

Is the underlying reason for the worker-related issues that constructor() returns a static v8::Persistent?

@rmnilin
Copy link
Author

rmnilin commented Sep 7, 2024

Thank you for the feedback!

Yes, the underlying issue with worker threads is related to the use of a static Nan::Persistent. This static handle could lead to race conditions and undefined behavior in a multi-threaded environment. By switching to v8::Eternal, we ensure that the constructor handle remains stable and safe across multiple threads, as v8::Eternal ties the handle's lifetime to the isolate, avoiding the problems associated with the static storage duration of Nan::Persistent.

While the primary distinction between Nan::Persistent and v8::Eternal is indeed the lifetime, v8::Eternal provides a more reliable guarantee for thread safety in multi-threaded environments, addressing the described issues.

I've also created rmnilin/ssh2-1393-repro to test both the current implementation and this fix.

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.

2 participants