Lock around encrypted I/Ocmn/lock-io

OpenSSL and often whatever libssh2 is using require their own set-up for
concurrent operations to be safe. This means that by default, using both
of these libraries in a threaded context is unsafe.

Lock by default, and allow the user to tell us that they've set up
threading for the underlying libraries.

9 files changed, 172 insertions, 10 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8a0ae0e03..9a3d03cf9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -49,6 +49,11 @@ support for HTTPS connections insead of OpenSSL.
   the error message, which allows you to get the "repository not
   found" messages.
 
+* OpenSSL and libssh2 code now locks by default as a way to have safe
+  defaults. Use `git_openssl_set_threadsafe()` and
+  `git_libssh2_set_threadsafe()` resp. to disable the locks if you
+  have made sure their locking is set up.
+
 
 ### API additions
 
@@ -98,6 +103,10 @@ support for HTTPS connections insead of OpenSSL.
   configuration of the server, and tools can use this to show messages
   about failing to communicate with the server.
 
+* `git_openssl_set_threadsafe()` and `git_libssh2_set_threadsafe()`
+  have been added to allow the user to specify that they have taken
+  care of setting up the threading for these libraries.
+
 ### API removals
 
 * `git_remote_save()` and `git_remote_clear_refspecs()` has been
diff --git a/THREADING.md b/THREADING.md
index cf7ac1ff0..e703a6c76 100644
--- a/THREADING.md
+++ b/THREADING.md
@@ -64,6 +64,13 @@ it should use. This means that libgit2 cannot know what to set as the
 user of libgit2 may use OpenSSL independently and the locking settings
 must survive libgit2 shutting down.
 
+As using these libraries without taking extra steps is unsafe, we err
+on the side of caution and lock around operations with these
+libraries. If you have set up thread safety for these librarires, you
+may call `git_openssl_set_threadafe()` and/or
+`git_libssh2_set_threadsafe()` which tell libgit2 that the libraries
+are thread-safe themselves and there is no need to take these locks.
+
 libgit2 does provide a last-resort convenience function
 `git_openssl_set_locking()` (available in `sys/openssl.h`) to use the
 platform-native mutex mechanisms to perform the locking, which you may
diff --git a/include/git2/sys/libssh2.h b/include/git2/sys/libssh2.h
new file mode 100644
index 000000000..ca1dec9fb
--- /dev/null
+++ b/include/git2/sys/libssh2.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) the libgit2 contributors. All rights reserved.
+ *
+ * This file is part of libgit2, distributed under the GNU GPL v2 with
+ * a Linking Exception. For full terms see the included COPYING file.
+ */
+#ifndef INCLUDE_git_libssh2_h__
+#define INCLUDE_git_libssh2_h__
+
+#include "git2/common.h"
+
+GIT_BEGIN_DECL
+
+/**
+ * Mark the libssh2 code as thread-safe
+ *
+ * By default we take a lock around libssh2 operations, as the
+ * thread-safety depends on the caller setting up the threading for
+ * the crytographic library it uses. If you have set up its threading,
+ * you may call this function to disable the lock, which would enable
+ * concurrent work.
+ *
+ * These locks are only used if the library was built with threading
+ * support.
+ */
+GIT_EXTERN(void) git_libssh2_set_threadsafe(void);
+
+GIT_END_DECL
+#endif
diff --git a/include/git2/sys/openssl.h b/include/git2/sys/openssl.h
index b41c55c6d..5a5fc8f03 100644
--- a/include/git2/sys/openssl.h
+++ b/include/git2/sys/openssl.h
@@ -28,11 +28,27 @@ GIT_BEGIN_DECL
  * likely sets up locking. You should very strongly prefer that over
  * this function.
  *
+ * This calls `git_openssl_set_threadsafe()` enabling concurrent
+ * OpenSSL operations.
+ *
  * @return 0 on success, -1 if there are errors or if libgit2 was not
  * built with OpenSSL and threading support.
  */
 GIT_EXTERN(int) git_openssl_set_locking(void);
 
+/**
+ * Mark the OpenSSL code as thread-safe
+ *
+ * By default we take a lock around OpenSSL operations. If you have
+ * set up OpenSSL threading in your application, you may call this
+ * function to avoid taking these locks, which would enable concurrent
+ * work.
+ *
+ * These locks are only used if the library was built with threading
+ * support.
+ */
+GIT_EXTERN(void) git_openssl_set_threadsafe(void);
+
 GIT_END_DECL
 #endif
 
diff --git a/src/global.c b/src/global.c
index 9f1a0bf10..f4005fa30 100644
--- a/src/global.c
+++ b/src/global.c
@@ -14,6 +14,7 @@
 
 
 git_mutex git__mwindow_mutex;
+git_mutex git__io_mutex;
 
 #define MAX_SHUTDOWN_CB 8
 
@@ -135,6 +136,7 @@ int git_openssl_set_locking(void)
 
 	CRYPTO_set_locking_callback(openssl_locking_function);
 	git__on_shutdown(shutdown_ssl_locking);
+	git_openssl_set_threadsafe();
 	return 0;
 # else
 	giterr_set(GITERR_THREAD, "libgit2 as not built with threads");
@@ -190,7 +192,8 @@ static int synchronized_threads_init(void)
 	int error;
 
 	_tls_index = TlsAlloc();
-	if (git_mutex_init(&git__mwindow_mutex))
+	if (git_mutex_init(&git__mwindow_mutex) ||
+	    git_mutex_init(&git__io_mutex))
 		return -1;
 
 	/* Initialize any other subsystems that have global state */
@@ -230,6 +233,7 @@ static void synchronized_threads_shutdown(void)
 
 	TlsFree(_tls_index);
 	git_mutex_free(&git__mwindow_mutex);
+	git_mutex_free(&git__io_mutex);
 }
 
 int git_libgit2_shutdown(void)
@@ -299,6 +303,8 @@ static void init_once(void)
 {
 	if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0)
 		return;
+	if ((init_error = git_mutex_init(&git__io_mutex)) != 0)
+		return;
 	pthread_key_create(&_tls_key, &cb__free_status);
 
 
@@ -342,6 +348,7 @@ int git_libgit2_shutdown(void)
 
 	pthread_key_delete(_tls_key);
 	git_mutex_free(&git__mwindow_mutex);
+	git_mutex_free(&git__io_mutex);
 	_once_init = new_once;
 
 	return 0;
diff --git a/src/global.h b/src/global.h
index fdad6ba89..93ba7ddc7 100644
--- a/src/global.h
+++ b/src/global.h
@@ -25,6 +25,7 @@ extern SSL_CTX *git__ssl_ctx;
 git_global_st *git__global_state(void);
 
 extern git_mutex git__mwindow_mutex;
+extern git_mutex git__io_mutex;
 
 #define GIT_GLOBAL (git__global_state())
 
diff --git a/src/openssl_stream.c b/src/openssl_stream.c
index 9b2d5951c..e3d63aef4 100644
--- a/src/openssl_stream.c
+++ b/src/openssl_stream.c
@@ -26,6 +26,17 @@
 #include <openssl/err.h>
 #include <openssl/x509v3.h>
 
+/*
+ * Whether we should take a lock around OpenSSL operations, the user
+ * can disable this if they've initialised OpenSSL's threading.
+ */
+static int should_lock = 1;
+
+void git_openssl_set_threadsafe(void)
+{
+	should_lock = 0;
+}
+
 static int ssl_set_error(SSL *ssl, int error)
 {
 	int err;
@@ -75,6 +86,8 @@ static int ssl_teardown(SSL *ssl)
 {
 	int ret;
 
+	LOCK;
+
 	ret = SSL_shutdown(ssl);
 	if (ret < 0)
 		ret = ssl_set_error(ssl, ret);
@@ -82,6 +95,9 @@ static int ssl_teardown(SSL *ssl)
 		ret = 0;
 
 	SSL_free(ssl);
+
+	UNLOCK;
+
 	return ret;
 }
 
@@ -109,7 +125,10 @@ static int verify_server_cert(SSL *ssl, const char *host)
 	void *addr;
 	int i = -1,j;
 
+	LOCK;
+
 	if (SSL_get_verify_result(ssl) != X509_V_OK) {
+		UNLOCK;
 		giterr_set(GITERR_SSL, "The SSL certificate is invalid");
 		return GIT_ECERTIFICATE;
 	}
@@ -128,6 +147,7 @@ static int verify_server_cert(SSL *ssl, const char *host)
 
 	cert = SSL_get_peer_certificate(ssl);
 	if (!cert) {
+		UNLOCK;
 		giterr_set(GITERR_SSL, "the server did not provide a certificate");
 		return -1;
 	}
@@ -167,8 +187,10 @@ static int verify_server_cert(SSL *ssl, const char *host)
 	if (matched == 0)
 		goto cert_fail_name;
 
-	if (matched == 1)
+	if (matched == 1) {
+		UNLOCK;
 		return 0;
+	}
 
 	/* If no alternative names are available, check the common name */
 	peer_name = X509_get_subject_name(cert);
@@ -213,10 +235,12 @@ static int verify_server_cert(SSL *ssl, const char *host)
 	return 0;
 
 on_error:
+	UNLOCK;
 	OPENSSL_free(peer_cn);
 	return ssl_set_error(ssl, 0);
 
 cert_fail_name:
+	UNLOCK;
 	OPENSSL_free(peer_cn);
 	giterr_set(GITERR_SSL, "hostname does not match certificate");
 	return GIT_ECERTIFICATE;
@@ -239,7 +263,10 @@ int openssl_connect(git_stream *stream)
 	if ((ret = git_stream_connect((git_stream *)st->socket)) < 0)
 		return ret;
 
+	LOCK;
+
 	if ((ret = SSL_set_fd(st->ssl, st->socket->s)) <= 0) {
+		UNLOCK;
 		openssl_close((git_stream *) st);
 		return ssl_set_error(st->ssl, ret);
 	}
@@ -247,7 +274,10 @@ int openssl_connect(git_stream *stream)
 	/* specify the host in case SNI is needed */
 	SSL_set_tlsext_host_name(st->ssl, st->socket->host);
 
-	if ((ret = SSL_connect(st->ssl)) <= 0)
+	ret = SSL_connect(st->ssl);
+	UNLOCK;
+
+	if (ret <= 0)
 		return ssl_set_error(st->ssl, ret);
 
 	return verify_server_cert(st->ssl, st->socket->host);
@@ -260,9 +290,12 @@ int openssl_certificate(git_cert **out, git_stream *stream)
 	X509 *cert = SSL_get_peer_certificate(st->ssl);
 	unsigned char *guard, *encoded_cert;
 
+	LOCK;
+
 	/* Retrieve the length of the certificate first */
 	len = i2d_X509(cert, NULL);
 	if (len < 0) {
+		UNLOCK;
 		giterr_set(GITERR_NET, "failed to retrieve certificate information");
 		return -1;
 	}
@@ -273,6 +306,7 @@ int openssl_certificate(git_cert **out, git_stream *stream)
 	guard = encoded_cert;
 
 	len = i2d_X509(cert, &guard);
+	UNLOCK;
 	if (len < 0) {
 		git__free(encoded_cert);
 		giterr_set(GITERR_NET, "failed to retrieve certificate information");
@@ -294,7 +328,10 @@ ssize_t openssl_write(git_stream *stream, const char *data, size_t len, int flag
 
 	GIT_UNUSED(flags);
 
-	if ((ret = SSL_write(st->ssl, data, len)) <= 0) {
+	LOCK;
+	ret = SSL_write(st->ssl, data, len);
+	UNLOCK;
+	if (ret <= 0) {
 		return ssl_set_error(st->ssl, ret);
 	}
 
@@ -306,9 +343,13 @@ ssize_t openssl_read(git_stream *stream, void *data, size_t len)
 	openssl_stream *st = (openssl_stream *) stream;
 	int ret;
 
+
+	LOCK;
 	if ((ret = SSL_read(st->ssl, data, len)) <= 0)
 		ssl_set_error(st->ssl, ret);
 
+	UNLOCK;
+
 	return ret;
 }
 
@@ -342,7 +383,9 @@ int git_openssl_stream_new(git_stream **out, const char *host, const char *port)
 	if (git_socket_stream_new((git_stream **) &st->socket, host, port))
 		return -1;
 
+	LOCK;
 	st->ssl = SSL_new(git__ssl_ctx);
+	UNLOCK;
 	if (st->ssl == NULL) {
 		giterr_set(GITERR_SSL, "failed to create ssl object");
 		return -1;
diff --git a/src/stream.h b/src/stream.h
index d810e704d..0a9fc187b 100644
--- a/src/stream.h
+++ b/src/stream.h
@@ -50,4 +50,14 @@ GIT_INLINE(void) git_stream_free(git_stream *st)
 	st->free(st);
 }
 
+/* Macros for locking around OpenSSL and libssh2; it assumes you have a static 'should_lock' variable */
+#define LOCK do {						   \
+	if (should_lock && git_mutex_lock(&git__io_mutex) < 0) {   \
+		giterr_set(GITERR_NET, "failed to lock IO mutex"); \
+		return -1;					   \
+	}							   \
+} while(0)
+
+#define UNLOCK git_mutex_unlock(&git__io_mutex)
+
 #endif
diff --git a/src/transports/ssh.c b/src/transports/ssh.c
index 55f715b1d..cc25dcc78 100644
--- a/src/transports/ssh.c
+++ b/src/transports/ssh.c
@@ -15,6 +15,15 @@
 #include "smart.h"
 #include "cred.h"
 #include "socket_stream.h"
+#include "global.h"
+#include "stream.h"
+
+static int should_lock = 1;
+
+void git_libssh2_set_threadsafe(void)
+{
+	should_lock = 0;
+}
 
 #ifdef GIT_SSH
 
@@ -97,7 +106,9 @@ static int send_command(ssh_stream *s)
 	if (error < 0)
 		goto cleanup;
 
+	LOCK;
 	error = libssh2_channel_exec(s->channel, request.ptr);
+	UNLOCK;
 	if (error < LIBSSH2_ERROR_NONE) {
 		ssh_error(s->session, "SSH could not execute request");
 		goto cleanup;
@@ -124,7 +135,10 @@ static int ssh_stream_read(
 	if (!s->sent_command && send_command(s) < 0)
 		return -1;
 
-	if ((rc = libssh2_channel_read(s->channel, buffer, buf_size)) < LIBSSH2_ERROR_NONE) {
+	LOCK;
+	rc = libssh2_channel_read(s->channel, buffer, buf_size);
+	UNLOCK;
+	if (rc < LIBSSH2_ERROR_NONE) {
 		ssh_error(s->session, "SSH could not read data");
 		return -1;
 	}
@@ -134,9 +148,14 @@ static int ssh_stream_read(
 	 * not-found error, so read from stderr and signal EOF on
 	 * stderr.
 	 */
-	if (rc == 0 && (rc = libssh2_channel_read_stderr(s->channel, buffer, buf_size)) > 0) {
-		giterr_set(GITERR_SSH, "%*s", rc, buffer);
-		return GIT_EEOF;
+	if (rc == 0) {
+		LOCK;
+		rc = libssh2_channel_read_stderr(s->channel, buffer, buf_size);
+		UNLOCK;
+		if (rc > 0) {
+			giterr_set(GITERR_SSH, "%*s", rc, buffer);
+			return GIT_EEOF;
+		}
 	}
 
 
@@ -158,7 +177,9 @@ static int ssh_stream_write(
 		return -1;
 
 	do {
+		LOCK;
 		ret = libssh2_channel_write(s->channel, buffer + off, len - off);
+		UNLOCK;
 		if (ret < 0)
 			break;
 
@@ -272,10 +293,15 @@ static int ssh_agent_auth(LIBSSH2_SESSION *session, git_cred_ssh_key *c) {
 
 	struct libssh2_agent_publickey *curr, *prev = NULL;
 
-	LIBSSH2_AGENT *agent = libssh2_agent_init(session);
+	LIBSSH2_AGENT *agent;
+
+	agent = libssh2_agent_init(session);
 
-	if (agent == NULL)
+	if (agent == NULL) {
+		UNLOCK;
+		giterr_set(GITERR_SSH, "failed to initialize agent connection");
 		return -1;
+	}
 
 	rc = libssh2_agent_connect(agent);
 
@@ -311,6 +337,7 @@ shutdown:
 
 	libssh2_agent_disconnect(agent);
 	libssh2_agent_free(agent);
+	UNLOCK;
 
 	return rc;
 }
@@ -321,6 +348,7 @@ static int _git_ssh_authenticate_session(
 {
 	int rc;
 
+	LOCK;
 	do {
 		giterr_clear();
 		switch (cred->credtype) {
@@ -374,6 +402,7 @@ static int _git_ssh_authenticate_session(
 			rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED;
 		}
 	} while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc);
+	UNLOCK;
 
         if (rc == LIBSSH2_ERROR_PASSWORD_EXPIRED || rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
                 return GIT_EAUTH;
@@ -434,8 +463,10 @@ static int _git_ssh_session_create(
 
 	assert(session);
 
+	LOCK;
 	s = libssh2_session_init();
 	if (!s) {
+		UNLOCK;
 		giterr_set(GITERR_NET, "Failed to initialize SSH session");
 		return -1;
 	}
@@ -443,6 +474,7 @@ static int _git_ssh_session_create(
 	do {
 		rc = libssh2_session_startup(s, socket->s);
 	} while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc);
+	UNLOCK;
 
 	if (rc != LIBSSH2_ERROR_NONE) {
 		ssh_error(s, "Failed to start SSH session");
@@ -495,6 +527,7 @@ static int _git_ssh_setup_conn(
 	    (error = git_stream_connect(s->io)) < 0)
 		goto done;
 
+	LOCK;
 	if ((error = _git_ssh_session_create(&session, s->io)) < 0)
 		goto done;
 
@@ -517,6 +550,7 @@ static int _git_ssh_setup_conn(
 		}
 
 		if (cert.type == 0) {
+			UNLOCK;
 			giterr_set(GITERR_SSH, "unable to get the host key");
 			error = -1;
 			goto done;
@@ -529,12 +563,14 @@ static int _git_ssh_setup_conn(
 
 		error = t->owner->certificate_check_cb((git_cert *) cert_ptr, 0, host, t->owner->message_cb_payload);
 		if (error < 0) {
+			UNLOCK;
 			if (!giterr_last())
 				giterr_set(GITERR_NET, "user cancelled hostkey check");
 
 			goto done;
 		}
 	}
+	UNLOCK;
 
 	/* we need the username to ask for auth methods */
 	if (!user) {
@@ -580,7 +616,9 @@ static int _git_ssh_setup_conn(
 	if (error < 0)
 		goto done;
 
+	LOCK;
 	channel = libssh2_channel_open_session(session);
+	UNLOCK;
 	if (!channel) {
 		error = -1;
 		ssh_error(session, "Failed to open SSH channel");
@@ -726,7 +764,9 @@ static int list_auth_methods(int *out, LIBSSH2_SESSION *session, const char *use
 
 	*out = 0;
 
+	LOCK;
 	list = libssh2_userauth_list(session, username, strlen(username));
+	UNLOCK;
 
 	/* either error, or the remote accepts NONE auth, which is bizarre, let's punt */
 	if (list == NULL && !libssh2_userauth_authenticated(session))