Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

OpenSSL and libssh2 thread safety#912

Merged
johnhaley81 merged 7 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:openssl-thread-safetysrajko/nodegit:openssl-thread-safetyCopy head branch name to clipboard
Feb 22, 2016
Merged

OpenSSL and libssh2 thread safety#912
johnhaley81 merged 7 commits intonodegit:masternodegit/nodegit:masterfrom
srajko:openssl-thread-safetysrajko/nodegit:openssl-thread-safetyCopy head branch name to clipboard

Conversation

@srajko
Copy link
Collaborator

@srajko srajko commented Feb 19, 2016

Provide openssl with the locking setup it needs for thread safety.

Initialize libssh2 to avoid the possibility of its subsequent thread-unsafe init.

@srajko srajko force-pushed the openssl-thread-safety branch 3 times, most recently from f1803c2 to 7670756 Compare February 19, 2016 22:45
@maxkorp maxkorp force-pushed the openssl-thread-safety branch from 0613231 to a38f96d Compare February 19, 2016 22:59
@srajko srajko force-pushed the openssl-thread-safety branch from a38f96d to 7302589 Compare February 19, 2016 23:18
@maxkorp maxkorp force-pushed the openssl-thread-safety branch 2 times, most recently from ab42426 to 75f618a Compare February 20, 2016 00:07
@maxkorp maxkorp force-pushed the openssl-thread-safety branch from 75f618a to b530b0f Compare February 20, 2016 00:10
@srajko
Copy link
Collaborator Author

srajko commented Feb 20, 2016

The libssh2_init fix resolves the crash from #907. Calls to fetch would initialize openssh sessions in separate threads, and those would in turn call libssh2_init via https://github.com/libssh2/libssh2/blob/d441da3086dcc49989b7c5b9bf3043c4a6a98211/src/global.c#L74-L78 which is not thread-safe. Calling libssh2_init at app initialization makes sure it's initialized in a thread-safe way.

Providing openssl with the callbacks it needs to operate in a thread safe manner also seems to help, and is probably something we should be doing anyway.

@srajko srajko force-pushed the openssl-thread-safety branch from b3f4c8c to a299094 Compare February 20, 2016 00:59
…conflicts

This is better than silencing the redefinition in case the two definitions really need to be different.
@srajko
Copy link
Collaborator Author

srajko commented Feb 22, 2016

There was a snag with doing the libssh2 initialization directly from nodegit.cc because node.h and libssh2.h had conflicting type definitions. The final fix for the problem was to move the libssh2.h include to a separate file and do the initialization from there.

@johnhaley81 johnhaley81 changed the title [WIP] openssl and libssh2 thread safety OpenSSL and libssh2 thread safety Feb 22, 2016
@johnhaley81
Copy link
Collaborator

LGTM

johnhaley81 added a commit that referenced this pull request Feb 22, 2016
OpenSSL and libssh2 thread safety
@johnhaley81 johnhaley81 merged commit f6cbd36 into nodegit:master Feb 22, 2016
@johnhaley81 johnhaley81 deleted the openssl-thread-safety branch February 22, 2016 17:41
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.