Use heap-bases contexts for MbedTLS handles (libevent#1355)

@widgetii:

"Recently after studying [https-client.c code](https://github.com/libevent/libevent/blob/master/sample/https-client.c#L532) I found that I cannot use MbedTLS with `bufferevent_mbedtls_socket_new` same way as for OpenSSL in other than hello-world code. In mentioned sample code, ssl context is created by `SSL_new()` (as heap-based pointer), but for MbedTLS stack value is used. The issue is in different semantics because OpenSSL is responsible for memory allocation and release for its context, but for MbedTLS it turns out user should do the same manually.

I expect that in both cases, setting option `BEV_OPT_CLOSE_ON_FREE` will free all linked resources, but in case of MbedTLS I have memory leak after connection is closed.

My proposal is:
1. Provide new `mbedtls_ssl_new` helper-function for end-user that do the same job as `SSL_new()` and use it and example in sample:

```c
mbedtls_ssl_context *mbedtls_ssl_new(const mbedtls_ssl_config *conf) {
  mbedtls_ssl_context *ssl = calloc(1, sizeof(*ssl));
  mbedtls_ssl_init(ssl);
  mbedtls_ssl_setup(ssl, conf);
  return ssl;
}
```

2. Add `free(ctx->ssl)` right after https://github.com/libevent/libevent/blob/master/bufferevent_mbedtls.c#L68"

Fixes: libevent#1354

bufferevent_mbedtls.c

@@ -47,7 +47,7 @@
 #include "mm-internal.h"
 
 struct mbedtls_context {
-	mbedtls_ssl_context *ssl;
+	mbedtls_dyncontext *ssl;
 	mbedtls_net_context net;
 };
 static void *
@@ -65,7 +65,7 @@ mbedtls_context_free(void *ssl, int flags)
 {
 	struct mbedtls_context *ctx = ssl;
 	if (flags & BEV_OPT_CLOSE_ON_FREE)
-		mbedtls_ssl_free(ctx->ssl);
+		bufferevent_mbedtls_dyncontext_free(ctx->ssl);
 	mm_free(ctx);
 }
 static int
@@ -309,7 +309,7 @@ bufferevent_get_mbedtls_error(struct bufferevent *bufev)
 static struct le_ssl_ops le_mbedtls_ops = {
 	mbedtls_context_init,
 	mbedtls_context_free,
-	(void (*)(void *))mbedtls_ssl_free,
+	(void (*)(void *))bufferevent_mbedtls_dyncontext_free,
 	mbedtls_context_renegotiate,
 	mbedtls_context_write,
 	mbedtls_context_read,
@@ -352,7 +352,7 @@ bufferevent_mbedtls_filter_new(struct event_base *base,
 
 err:
 	if (options & BEV_OPT_CLOSE_ON_FREE)
-		mbedtls_ssl_free(ssl);
+		bufferevent_mbedtls_dyncontext_free(ssl);
 	return NULL;
 }
 
@@ -407,3 +407,19 @@ bufferevent_mbedtls_socket_new(struct event_base *base, evutil_socket_t fd,
 err:
 	return NULL;
 }
+
+mbedtls_dyncontext *
+bufferevent_mbedtls_dyncontext_new(struct mbedtls_ssl_config *conf)
+{
+	mbedtls_dyncontext *ctx = mm_calloc(1, sizeof(*ctx));
+	mbedtls_ssl_init(ctx);
+	mbedtls_ssl_setup(ctx, conf);
+	return ctx;
+}
+
+void
+bufferevent_mbedtls_dyncontext_free(mbedtls_dyncontext *ctx)
+{
+	mbedtls_ssl_free(ctx);
+	mm_free(ctx);
+}

include/event2/bufferevent_ssl.h

@@ -183,6 +183,9 @@ unsigned long bufferevent_get_openssl_error(struct bufferevent *bev);
 #endif
 #if defined(EVENT__HAVE_MBEDTLS) || defined(EVENT_IN_DOXYGEN_)
 struct mbedtls_ssl_context;
+struct mbedtls_ssl_config;
+typedef struct mbedtls_ssl_context mbedtls_dyncontext;
+
 /**
    Create a new SSL bufferevent to send its data over another bufferevent.
 
@@ -198,7 +201,7 @@ EVENT2_EXPORT_SYMBOL
 struct bufferevent *
 bufferevent_mbedtls_filter_new(struct event_base *base,
     struct bufferevent *underlying,
-    struct mbedtls_ssl_context *ssl,
+    mbedtls_dyncontext *ssl,
     enum bufferevent_ssl_state state,
     int options);
 
@@ -216,7 +219,7 @@ EVENT2_EXPORT_SYMBOL
 struct bufferevent *
 bufferevent_mbedtls_socket_new(struct event_base *base,
     evutil_socket_t fd,
-    struct mbedtls_ssl_context *ssl,
+    mbedtls_dyncontext *ssl,
     enum bufferevent_ssl_state state,
     int options);
 
@@ -249,10 +252,20 @@ bufferevent_mbedtls_get_ssl(struct bufferevent *bufev);
 EVENT2_EXPORT_SYMBOL
 int bufferevent_mbedtls_renegotiate(struct bufferevent *bev);
 
-/** Return the most recent OpenSSL error reported on an SSL bufferevent. */
+/** Return the most recent MbedTLS error reported on an SSL bufferevent. */
 EVENT2_EXPORT_SYMBOL
 unsigned long bufferevent_get_mbedtls_error(struct bufferevent *bev);
 
+/** Create a new heap-based MbedTLS context for use it in bufferevent_mbedtls_* functions */
+EVENT2_EXPORT_SYMBOL
+mbedtls_dyncontext *
+bufferevent_mbedtls_dyncontext_new(struct mbedtls_ssl_config *conf);
+
+/** Deallocate heap-based MbedTLS context */
+EVENT2_EXPORT_SYMBOL
+void
+bufferevent_mbedtls_dyncontext_free(mbedtls_dyncontext *ctx);
+
 #endif
 
 #ifdef __cplusplus

sample/https-client.c

@@ -274,7 +274,7 @@ main(int argc, char **argv)
 	int timeout = -1;
 
 #ifdef USE_MBEDTLS
-	mbedtls_ssl_context ssl;
+	mbedtls_dyncontext* ssl = NULL;
 	mbedtls_ssl_config config;
 	mbedtls_ctr_drbg_context ctr_drbg;
 	mbedtls_entropy_context entropy;
@@ -297,7 +297,6 @@ main(int argc, char **argv)
 	mbedtls_ctr_drbg_init(&ctr_drbg);
 	mbedtls_entropy_init(&entropy);
 	mbedtls_ssl_config_init(&config);
-	mbedtls_ssl_init(&ssl);
 #else
 	enum { HTTP, HTTPS } type = HTTP;
 #endif
@@ -428,7 +427,7 @@ main(int argc, char **argv)
 		mbedtls_ssl_conf_ca_chain(&config, &cacert, NULL);
 	}
 
-	mbedtls_ssl_setup(&ssl, &config);
+	ssl = bufferevent_mbedtls_dyncontext_new(&config);
 #else
 #if (OPENSSL_VERSION_NUMBER < 0x10100000L) || \
 	(defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x20700000L)
@@ -510,7 +509,7 @@ main(int argc, char **argv)
 	}
 
 #ifdef USE_MBEDTLS
-	mbedtls_ssl_set_hostname(&ssl, host);
+	mbedtls_ssl_set_hostname(ssl, host);
 #else
 	// Create OpenSSL bufferevent and stack evhttp on top of it
 	ssl = SSL_new(ssl_ctx);
@@ -528,16 +527,15 @@ main(int argc, char **argv)
 	if (strcasecmp(scheme, "http") == 0) {
 		bev = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE);
 	} else {
-#ifdef USE_MBEDTLS
-		bev = bufferevent_mbedtls_socket_new(base, -1, &ssl,
-											 BUFFEREVENT_SSL_CONNECTING,
-											 BEV_OPT_CLOSE_ON_FREE|BEV_OPT_DEFER_CALLBACKS);
-#else
+#ifndef USE_MBEDTLS
 		type = HTTPS;
-		bev = bufferevent_openssl_socket_new(base, -1, ssl,
+		bev = bufferevent_openssl_socket_new(
+#else
+		bev = bufferevent_mbedtls_socket_new(
+#endif
+			base, -1, ssl,
 			BUFFEREVENT_SSL_CONNECTING,
 			BEV_OPT_CLOSE_ON_FREE|BEV_OPT_DEFER_CALLBACKS);
-#endif
 	}
 
 	if (bev == NULL) {
@@ -639,7 +637,6 @@ main(int argc, char **argv)
 		event_base_free(base);
 
 #ifdef USE_MBEDTLS
-	mbedtls_ssl_free(&ssl);
 	mbedtls_ssl_config_free(&config);
 	mbedtls_ctr_drbg_free(&ctr_drbg);
 	mbedtls_x509_crt_free(&cacert);

sample/ssl-client-mbedtls.c

@@ -146,7 +146,7 @@ main(void)
 
 	mbedtls_entropy_context entropy;
 	mbedtls_ctr_drbg_context ctr_drbg;
-	mbedtls_ssl_context ssl;
+	mbedtls_dyncontext* ssl;
 	mbedtls_ssl_config conf;
 	mbedtls_x509_crt cacert;
 
@@ -175,7 +175,6 @@ main(void)
 	 * 0. Initialize the RNG and the session data
 	 */
 	mbedtls_net_init(&server_fd);
-	mbedtls_ssl_init(&ssl);
 	mbedtls_ssl_config_init(&conf);
 	mbedtls_x509_crt_init(&cacert);
 	mbedtls_ctr_drbg_init(&ctr_drbg);
@@ -244,12 +243,9 @@ main(void)
 	mbedtls_ssl_conf_rng(&conf, mbedtls_ctr_drbg_random, &ctr_drbg);
 	mbedtls_ssl_conf_dbg(&conf, my_debug, stdout);
 
-	if ((ret = mbedtls_ssl_setup(&ssl, &conf)) != 0) {
-		mbedtls_printf(" failed\n  ! mbedtls_ssl_setup returned %d\n\n", ret);
-		goto exit;
-	}
+	ssl = bufferevent_mbedtls_dyncontext_new(&conf);
 
-	if ((ret = mbedtls_ssl_set_hostname(&ssl, SERVER_NAME)) != 0) {
+	if ((ret = mbedtls_ssl_set_hostname(ssl, SERVER_NAME)) != 0) {
 		mbedtls_printf(
 			" failed\n  ! mbedtls_ssl_set_hostname returned %d\n\n", ret);
 		goto exit;
@@ -265,7 +261,7 @@ main(void)
 
 	bev = bufferevent_socket_new(evbase, server_fd.fd, BEV_OPT_CLOSE_ON_FREE);
 	bevf = bufferevent_mbedtls_filter_new(
-		evbase, bev, &ssl, BUFFEREVENT_SSL_CONNECTING, BEV_OPT_CLOSE_ON_FREE);
+		evbase, bev, ssl, BUFFEREVENT_SSL_CONNECTING, BEV_OPT_CLOSE_ON_FREE);
 	bev = bevf;
 	bufferevent_setcb(bev, readcb, writecb, eventcb, NULL);
 
@@ -289,7 +285,6 @@ main(void)
 	mbedtls_net_free(&server_fd);
 
 	mbedtls_x509_crt_free(&cacert);
-	mbedtls_ssl_free(&ssl);
 	mbedtls_ssl_config_free(&conf);
 	mbedtls_ctr_drbg_free(&ctr_drbg);
 	mbedtls_entropy_free(&entropy);

test-export/test-export.c

@@ -99,7 +99,7 @@ test()
 {
 	struct event_base *base = NULL;
 	mbedtls_ssl_config *conf = NULL;
-	mbedtls_ssl_context *ssl = NULL;
+	mbedtls_dyncontext *ssl = NULL;
 	struct bufferevent *bev;
 	int r = 1;
 
@@ -114,12 +114,7 @@ test()
 	}
 	mbedtls_ssl_config_init(conf);
 
-	ssl = malloc(sizeof(*ssl));
-	if (!ssl) {
-		goto error;
-	}
-	mbedtls_ssl_init(ssl);
-	mbedtls_ssl_setup(ssl, conf);
+	ssl = bufferevent_mbedtls_dyncontext_new(conf);
 
 	bev = bufferevent_mbedtls_socket_new(base, -1, ssl,
 		BUFFEREVENT_SSL_CONNECTING,
@@ -132,8 +127,7 @@ test()
 	if (base)
 		event_base_free(base);
 	if (ssl) {
-		mbedtls_ssl_free(ssl);
-		free(ssl);
+		bufferevent_mbedtls_dyncontext_free(ssl);
 	}
 	if (conf) {
 		mbedtls_ssl_config_free(conf);

test/regress.h

@@ -146,7 +146,6 @@ void init_ssl(void);
 #ifdef EVENT__HAVE_MBEDTLS
 #include <mbedtls/ssl.h>
 mbedtls_ssl_config *get_mbedtls_config(int endpoint);
-mbedtls_ssl_context *mbedtls_ssl_new(mbedtls_ssl_config *config);
 #endif
 
 void * basic_test_setup(const struct testcase_t *testcase);

test/regress_http.c

@@ -168,7 +168,7 @@ https_bev(struct event_base *base, void *arg)
 static struct bufferevent *
 https_mbedtls_bev(struct event_base *base, void *arg)
 {
-	mbedtls_ssl_context *ssl = mbedtls_ssl_new(get_mbedtls_config(MBEDTLS_SSL_IS_SERVER));
+	mbedtls_dyncontext *ssl = bufferevent_mbedtls_dyncontext_new(get_mbedtls_config(MBEDTLS_SSL_IS_SERVER));
 	return bufferevent_mbedtls_socket_new(
 		base, -1, ssl, BUFFEREVENT_SSL_ACCEPTING,
 		BEV_OPT_CLOSE_ON_FREE);
@@ -558,7 +558,7 @@ create_bev(struct event_base *base, evutil_socket_t fd, int ssl_mask, int flags_
 #endif
 	} else if (ssl_mask & HTTP_MBEDTLS) {
 #ifdef EVENT__HAVE_MBEDTLS
-		mbedtls_ssl_context *ssl = mbedtls_ssl_new(get_mbedtls_config(MBEDTLS_SSL_IS_CLIENT));
+		mbedtls_dyncontext *ssl = bufferevent_mbedtls_dyncontext_new(get_mbedtls_config(MBEDTLS_SSL_IS_CLIENT));
 		if (ssl_mask & HTTP_SSL_FILTER) {
 			struct bufferevent *underlying =
 			bufferevent_socket_new(base, fd, flags);
@@ -3615,7 +3615,7 @@ http_incomplete_test_(struct basic_test_data *data, int use_timeout, int ssl)
 	tt_assert(fd != EVUTIL_INVALID_SOCKET);
 
 	/* Stupid thing to send a request */
-	bev = create_bev(data->base, fd, ssl, 0);
+	bev = create_bev(data->base, fd, ssl, BEV_OPT_CLOSE_ON_FREE);
 	bufferevent_setcb(bev,
 	    http_incomplete_readcb, http_incomplete_writecb,
 	    http_incomplete_errorcb, use_timeout ? NULL : &fd);
@@ -4007,7 +4007,7 @@ http_stream_out_test_impl(void *arg, int ssl)
 	test_ok = 0;
 	exit_base = data->base;
 
-	bev = create_bev(data->base, -1, ssl, 0);
+	bev = create_bev(data->base, -1, ssl, BEV_OPT_CLOSE_ON_FREE);
 	evcon = evhttp_connection_base_bufferevent_new(
 		data->base, NULL, bev, "127.0.0.1", port);
 	tt_assert(evcon);
@@ -4207,7 +4207,7 @@ http_connection_fail_test_impl(void *arg, int ssl)
 	/* auto detect a port */
 	evhttp_free(http);
 
-	bev = create_bev(data->base, -1, ssl, 0);
+	bev = create_bev(data->base, -1, ssl, BEV_OPT_CLOSE_ON_FREE);
 	/* Pick an unroutable address. This administratively scoped multicast
 	 * address should do when working with TCP. */
 	evcon = evhttp_connection_base_bufferevent_new(
@@ -4279,7 +4279,7 @@ http_simple_test_impl(void *arg, int ssl, int dirty, const char *uri)
 	exit_base = data->base;
 	test_ok = 0;
 
-	bev = create_bev(data->base, -1, ssl, 0);
+	bev = create_bev(data->base, -1, ssl, BEV_OPT_CLOSE_ON_FREE);
 #ifdef EVENT__HAVE_OPENSSL
 	bufferevent_openssl_set_allow_dirty_shutdown(bev, dirty);
 #endif
@@ -4359,7 +4359,7 @@ https_per_socket_bevcb_impl(void *arg, ev_uint16_t http_port, ev_uint16_t https_
 
 	evhttp_set_gencb(http, http_basic_cb, http);
 
-	bev = create_bev(data->base, -1, mask, 0);
+	bev = create_bev(data->base, -1, mask, BEV_OPT_CLOSE_ON_FREE);
 
 #ifdef EVENT__HAVE_OPENSSL
 	bufferevent_openssl_set_allow_dirty_shutdown(bev, 1);
@@ -5204,7 +5204,7 @@ http_write_during_read_test_impl(void *arg, int ssl)
 
 	fd = http_connect("127.0.0.1", port);
 	tt_assert(fd != EVUTIL_INVALID_SOCKET);
-	bev = create_bev(data->base, fd, ssl, 0);
+	bev = create_bev(data->base, fd, ssl, BEV_OPT_CLOSE_ON_FREE);
 	bufferevent_setcb(bev, NULL, NULL, NULL, data->base);
 	bufferevent_disable(bev, EV_READ);
 

test/regress_mbedtls.c

@@ -51,7 +51,7 @@
 #undef SSL_get_peer_certificate
 #define SSL_get_peer_certificate mbedtls_ssl_get_peer_cert
 #define SSL_get1_peer_certificate mbedtls_ssl_get_peer_cert
-#define SSL_new mbedtls_ssl_new
+#define SSL_new bufferevent_mbedtls_dyncontext_new
 #define SSL_use_certificate(a, b) \
 	do {                          \
 	} while (0);
@@ -80,8 +80,6 @@ const struct testcase_setup_t mbedtls_setup = {
 #define ssl_setup mbedtls_setup
 #include "regress_ssl.c"
 static mbedtls_ssl_config *the_mbedtls_conf[2] = {NULL, NULL};
-static mbedtls_ssl_context *the_mbedtls_ctx[1024] = {NULL};
-static int the_mbedtls_ctx_count = 0;
 static mbedtls_entropy_context entropy;
 static mbedtls_ctr_drbg_context ctr_drbg;
 static mbedtls_x509_crt *the_cert;
@@ -282,7 +280,6 @@ mbedtls_test_setup(const struct testcase_t *testcase)
 static int
 mbedtls_test_cleanup(const struct testcase_t *testcase, void *ptr)
 {
-	int i;
 	int ret = basic_test_cleanup(testcase, ptr);
 	if (!ret) {
 		return ret;
@@ -303,9 +300,6 @@ mbedtls_test_cleanup(const struct testcase_t *testcase, void *ptr)
 	mbedtls_pk_free(the_key);
 	free(the_key);
 
-	for (i = 0; i < the_mbedtls_ctx_count; i++) {
-		mbedtls_ssl_free(the_mbedtls_ctx[i]);
-	}
 	if (the_mbedtls_conf[0]) {
 		mbedtls_ssl_config_free(the_mbedtls_conf[0]);
 		free(the_mbedtls_conf[0]);
@@ -320,16 +314,6 @@ mbedtls_test_cleanup(const struct testcase_t *testcase, void *ptr)
 	return 1;
 }
 
-mbedtls_ssl_context *
-mbedtls_ssl_new(mbedtls_ssl_config *config)
-{
-	mbedtls_ssl_context *ssl = malloc(sizeof(*ssl));
-	mbedtls_ssl_init(ssl);
-	mbedtls_ssl_setup(ssl, config);
-	the_mbedtls_ctx[the_mbedtls_ctx_count++] = ssl;
-	return ssl;
-}
-
 static int
 bio_rwcount_read(void *ctx, unsigned char *out, size_t outlen)
 {