Merge pull request #5398 from libgit2/pks/valgrind-openssl

openssl: fix Valgrind issues in nightly builds

4 files changed, 61 insertions, 31 deletions

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8765a97b5..7f001d3d1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -58,7 +58,6 @@ OPTION(USE_SHA1				"Enable SHA1. Can be set to CollisionDetection(ON)/HTTPS/Gene
 OPTION(USE_GSSAPI			"Link with libgssapi for SPNEGO auth"			OFF)
 OPTION(USE_STANDALONE_FUZZERS		"Enable standalone fuzzers (compatible with gcc)"	OFF)
 OPTION(USE_LEAK_CHECKER			"Run tests with leak checker"				OFF)
-OPTION(VALGRIND				"Configure build for valgrind"				OFF)
 OPTION(DEBUG_POOL			"Enable debug pool allocator"				OFF)
 OPTION(ENABLE_WERROR			"Enable compilation with -Werror"			OFF)
 OPTION(USE_BUNDLED_ZLIB    		"Use the bundled version of zlib"			OFF)
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 2b593dd2c..2575f475f 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -19,7 +19,7 @@ jobs:
       environmentVariables: |
        CC=gcc
        CMAKE_GENERATOR=Ninja
-       CMAKE_OPTIONS=-DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DVALGRIND=on -DUSE_GSSAPI=ON
+       CMAKE_OPTIONS=-DUSE_HTTPS=OpenSSL -DREGEX_BACKEND=builtin -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON
        GITTEST_NEGOTIATE_PASSWORD=${{ variables.GITTEST_NEGOTIATE_PASSWORD }}
 
 - job: linux_amd64_xenial_gcc_mbedtls
@@ -35,7 +35,7 @@ jobs:
       environmentVariables: |
        CC=gcc
        CMAKE_GENERATOR=Ninja
-       CMAKE_OPTIONS=-DUSE_HTTPS=mbedTLS -DUSE_SHA1=HTTPS -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DVALGRIND=on -DUSE_GSSAPI=ON
+       CMAKE_OPTIONS=-DUSE_HTTPS=mbedTLS -DUSE_SHA1=HTTPS -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON
        GITTEST_NEGOTIATE_PASSWORD=${{ variables.GITTEST_NEGOTIATE_PASSWORD }}
 
 - job: linux_amd64_xenial_clang_openssl
@@ -51,7 +51,7 @@ jobs:
       environmentVariables: |
        CC=clang
        CMAKE_GENERATOR=Ninja
-       CMAKE_OPTIONS=-DUSE_HTTPS=OpenSSL -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DVALGRIND=on -DUSE_GSSAPI=ON
+       CMAKE_OPTIONS=-DUSE_HTTPS=OpenSSL -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON
        GITTEST_NEGOTIATE_PASSWORD=${{ variables.GITTEST_NEGOTIATE_PASSWORD }}
 
 - job: linux_amd64_xenial_clang_mbedtls
@@ -67,7 +67,7 @@ jobs:
       environmentVariables: |
        CC=clang
        CMAKE_GENERATOR=Ninja
-       CMAKE_OPTIONS=-DUSE_HTTPS=mbedTLS -DUSE_SHA1=HTTPS -DREGEX_BACKEND=pcre -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DVALGRIND=on -DUSE_GSSAPI=ON
+       CMAKE_OPTIONS=-DUSE_HTTPS=mbedTLS -DUSE_SHA1=HTTPS -DREGEX_BACKEND=pcre -DDEPRECATE_HARD=ON -DUSE_LEAK_CHECKER=valgrind -DUSE_GSSAPI=ON
        GITTEST_NEGOTIATE_PASSWORD=${{ variables.GITTEST_NEGOTIATE_PASSWORD }}
 
 - job: macos
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 9e4a41a61..bcc6a0139 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -288,14 +288,13 @@ IF (WIN32 AND NOT CYGWIN)
 ELSEIF (AMIGA)
 	ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R -DNO_MMAP)
 ELSE()
-	ADD_FEATURE_INFO(valgrind VALGRIND "valgrind hints")
-	IF (VALGRIND)
-		ADD_DEFINITIONS(-DVALGRIND)
-	ENDIF()
-
 	FILE(GLOB SRC_OS unix/*.c unix/*.h)
 ENDIF()
 
+IF (USE_LEAK_CHECKER STREQUAL "valgrind")
+	ADD_DEFINITIONS(-DVALGRIND)
+ENDIF()
+
 FILE(GLOB SRC_GIT2 *.c *.h
 	allocators/*.c allocators/*.h
 	streams/*.c streams/*.h
diff --git a/src/streams/openssl.c b/src/streams/openssl.c
index 98a3635af..458810c91 100644
--- a/src/streams/openssl.c
+++ b/src/streams/openssl.c
@@ -30,10 +30,6 @@
 #include <openssl/x509v3.h>
 #include <openssl/bio.h>
 
-#ifdef VALGRIND
-# include <valgrind/memcheck.h>
-#endif
-
 SSL_CTX *git__ssl_ctx;
 
 #define GIT_SSL_DEFAULT_CIPHERS "ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-DSS-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA:DHE-DSS-AES128-SHA256:DHE-DSS-AES256-SHA256:DHE-DSS-AES128-SHA:DHE-DSS-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA"
@@ -200,16 +196,69 @@ static void shutdown_ssl(void)
 	}
 }
 
+#ifdef VALGRIND
+#ifdef OPENSSL_LEGACY_API
+static void *git_openssl_malloc(size_t bytes)
+{
+	return git__calloc(1, bytes);
+}
+
+static void *git_openssl_realloc(void *mem, size_t size)
+{
+	return git__realloc(mem, size);
+}
+
+static void git_openssl_free(void *mem)
+{
+	return git__free(mem);
+}
+#else
+static void *git_openssl_malloc(size_t bytes, const char *file, int line)
+{
+	GIT_UNUSED(file);
+	GIT_UNUSED(line);
+	return git__calloc(1, bytes);
+}
+
+static void *git_openssl_realloc(void *mem, size_t size, const char *file, int line)
+{
+	GIT_UNUSED(file);
+	GIT_UNUSED(line);
+	return git__realloc(mem, size);
+}
+
+static void git_openssl_free(void *mem, const char *file, int line)
+{
+	GIT_UNUSED(file);
+	GIT_UNUSED(line);
+	return git__free(mem);
+}
+#endif
+#endif
+
 int git_openssl_stream_global_init(void)
 {
 	long ssl_opts = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
 	const char *ciphers = git_libgit2__ssl_ciphers();
+#ifdef VALGRIND
+	static bool allocators_initialized = false;
+#endif
 
 	/* Older OpenSSL and MacOS OpenSSL doesn't have this */
 #ifdef SSL_OP_NO_COMPRESSION
 	ssl_opts |= SSL_OP_NO_COMPRESSION;
 #endif
 
+#ifdef VALGRIND
+	/* Swap in our own allocator functions that initialize allocated memory */
+	if (!allocators_initialized &&
+	    CRYPTO_set_mem_functions(git_openssl_malloc,
+				     git_openssl_realloc,
+				     git_openssl_free) != 1)
+		goto error;
+	allocators_initialized = true;
+#endif
+
 	OPENSSL_init_ssl(0, NULL);
 
 	/*
@@ -314,11 +363,6 @@ static int bio_read(BIO *b, char *buf, int len)
 static int bio_write(BIO *b, const char *buf, int len)
 {
 	git_stream *io = (git_stream *) BIO_get_data(b);
-
-#ifdef VALGRIND
-	VALGRIND_MAKE_MEM_DEFINED(buf, len);
-#endif
-
 	return (int) git_stream_write(io, buf, len, 0);
 }
 
@@ -595,10 +639,6 @@ static int openssl_connect(git_stream *stream)
 	BIO_set_data(bio, st->io);
 	SSL_set_bio(st->ssl, bio, bio);
 
-#ifdef VALGRIND
-	VALGRIND_MAKE_MEM_DEFINED(st->ssl, sizeof(SSL));
-#endif
-
 	/* specify the host in case SNI is needed */
 #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
 	SSL_set_tlsext_host_name(st->ssl, st->host);
@@ -609,10 +649,6 @@ static int openssl_connect(git_stream *stream)
 
 	st->connected = true;
 
-#ifdef VALGRIND
-	VALGRIND_MAKE_MEM_DEFINED(st->ssl, sizeof(SSL));
-#endif
-
 	return verify_server_cert(st->ssl, st->host);
 }
 
@@ -679,10 +715,6 @@ static ssize_t openssl_read(git_stream *stream, void *data, size_t len)
 	if ((ret = SSL_read(st->ssl, data, len)) <= 0)
 		return ssl_set_error(st->ssl, ret);
 
-#ifdef VALGRIND
-	VALGRIND_MAKE_MEM_DEFINED(data, ret);
-#endif
-
 	return ret;
 }