[Tarantool-patches] [PATCH v1] asan: leak unit/swim.test:swim_test_encryption

[Tarantool-patches] [PATCH v1] asan: leak unit/swim.test:swim_test_encryption

[Alexander V. Tikhonov]

Found leak issue:

  [001] +==41031==ERROR: LeakSanitizer: detected memory leaks
  [001] +
  [001] +Direct leak of 96 byte(s) in 2 object(s) allocated from:
  [001] +    #0 0x4d8e53 in __interceptor_malloc (/tnt/test/unit/swim.test+0x4d8e53)
  [001] +    #1 0x53560f in crypto_codec_new /source/src/lib/crypto/crypto.c:239:51
  [001] +    #2 0x5299c4 in swim_scheduler_set_codec /source/src/lib/swim/swim_io.c:700:30
  [001] +    #3 0x511fe6 in swim_cluster_set_codec /source/test/unit/swim_test_utils.c:251:2
  [001] +    #4 0x50b3ae in swim_test_encryption /source/test/unit/swim.c:767:2
  [001] +    #5 0x50b3ae in main_f /source/test/unit/swim.c:1123
  [001] +    #6 0x544a3b in fiber_loop /source/src/lib/core/fiber.c:869:18
  [001] +    #7 0x5a13d0 in coro_init /source/third_party/coro/coro.c:110:3
  [001] +
  [001] +SUMMARY: AddressSanitizer: 96 byte(s) leaked in 2 allocation(s).

Prepared minimal issue reproducer:

  static void
  swim_test_encryption(void)
  {
          swim_start_test(3);
          struct swim_cluster *cluster = swim_cluster_new(2);
          swim_cluster_set_codec(cluster, CRYPTO_ALGO_AES128, CRYPTO_MODE_CBC,
                                 "1234567812345678", CRYPTO_AES128_KEY_SIZE);
          swim_cluster_delete(cluster);
          swim_finish_test();
  }

Found that memory allocated for codec creation at crypto_codec_new()
using swim_cluster_set_codec(). But there was no any memory free for
it. Decided to integrate at test utilities swim_cluster_delete_codec()
function for it available for tests.

After this fix removed susspencion on memory leak for unit/swin.test.

Closes #5283
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5283-asan-swim
Issue: https://github.com/tarantool/tarantool/issues/5283

 asan/lsan.supp              |  4 ----
 src/lib/swim/swim.c         |  6 ++++++
 src/lib/swim/swim.h         |  9 +++++++++
 src/lib/swim/swim_io.c      |  8 ++++++++
 src/lib/swim/swim_io.h      |  4 ++++
 test/unit/swim.c            |  4 +++-
 test/unit/swim_test_utils.c | 14 ++++++++++++++
 test/unit/swim_test_utils.h |  7 +++++++
 8 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/asan/lsan.supp b/asan/lsan.supp
index 77b7c2cfc..46b3001e9 100644
--- a/asan/lsan.supp
+++ b/asan/lsan.supp
@@ -52,10 +52,6 @@ leak:mh_i32ptr_new
 # source: src/lib/core/exception.cc
 leak:Exception::operator new
 
-# test: unit/swim.test.lua
-# source: src/lib/swim/swim_io.c
-leak:swim_scheduler_set_codec
-
 # test: vinyl/errinj.test.lua
 # source: src/lib/core/fiber.h
 leak:fiber_cxx_invoke
diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 396bd7c45..554e16efe 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -2114,6 +2114,12 @@ swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
 					key, key_size);
 }
 
+int
+swim_delete_codec(struct swim *swim)
+{
+       return swim_scheduler_delete_codec(&swim->scheduler);
+}
+
 bool
 swim_is_configured(const struct swim *swim)
 {
diff --git a/src/lib/swim/swim.h b/src/lib/swim/swim.h
index 4565eb976..e684205c4 100644
--- a/src/lib/swim/swim.h
+++ b/src/lib/swim/swim.h
@@ -124,6 +124,15 @@ int
 swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
 	       const char *key, int key_size);
 
+
+/**
+ * Delete SWIM codec used to encrypt/decrypt messages.
+ * @param swim SWIM instance to set codec for.
+ */
+int
+swim_delete_codec(struct swim *swim);
+
+
 /**
  * Stop listening and broadcasting messages, cleanup all internal
  * structures, free memory. The function yields. Actual deletion
diff --git a/src/lib/swim/swim_io.c b/src/lib/swim/swim_io.c
index af1092416..9fc2dc604 100644
--- a/src/lib/swim/swim_io.c
+++ b/src/lib/swim/swim_io.c
@@ -708,6 +708,14 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
 	return 0;
 }
 
+int
+swim_scheduler_delete_codec(struct swim_scheduler *scheduler)
+{
+       if (scheduler->codec != NULL)
+               crypto_codec_delete(scheduler->codec);
+       return 0;
+}
+
 const char *
 swim_inaddr_str(const struct sockaddr_in *addr)
 {
diff --git a/src/lib/swim/swim_io.h b/src/lib/swim/swim_io.h
index bf5a1389f..72c6c030d 100644
--- a/src/lib/swim/swim_io.h
+++ b/src/lib/swim/swim_io.h
@@ -204,6 +204,10 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
 			 enum crypto_algo algo, enum crypto_mode mode,
 			 const char *key, int key_size);
 
+/** Delete the codec used to encrypt/decrypt messages. */
+int
+swim_scheduler_delete_codec(struct swim_scheduler *scheduler);
+
 /** Stop accepting new packets from the network. */
 void
 swim_scheduler_stop_input(struct swim_scheduler *scheduler);
diff --git a/test/unit/swim.c b/test/unit/swim.c
index bb12baf8d..5c9ae618b 100644
--- a/test/unit/swim.c
+++ b/test/unit/swim.c
@@ -797,6 +797,8 @@ swim_test_encryption(void)
 	is(swim_cluster_wait_fullmesh(cluster, 2), 0,
 	   "cluster works after encryption has been disabled");
 
+	swim_cluster_delete_codec(cluster);
+
 	swim_cluster_delete(cluster);
 
 	swim_finish_test();
@@ -1140,4 +1142,4 @@ main()
 {
 	swim_run_test("swim.txt", main_f);
 	return test_result;
-}
\ No newline at end of file
+}
diff --git a/test/unit/swim_test_utils.c b/test/unit/swim_test_utils.c
index 9dbd28a9f..0eeb85fa4 100644
--- a/test/unit/swim_test_utils.c
+++ b/test/unit/swim_test_utils.c
@@ -237,6 +237,14 @@ swim_cluster_new(int size)
 	}								\
 })
 
+#define swim_cluster_set_cfg_noargs(cluster, func) ({                          \
+       for (int i = 0; i < cluster->size; ++i) {                       \
+               int rc = func(cluster->node[i].swim);   \
+               assert(rc == 0);                                        \
+               (void) rc;                                              \
+       }                                                               \
+})
+
 void
 swim_cluster_set_ack_timeout(struct swim_cluster *cluster, double ack_timeout)
 {
@@ -252,6 +260,12 @@ swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
 			     key, key_size);
 }
 
+void
+swim_cluster_delete_codec(struct swim_cluster *cluster)
+{
+       swim_cluster_set_cfg_noargs(cluster, swim_delete_codec);
+}
+
 void
 swim_cluster_set_gc(struct swim_cluster *cluster, enum swim_gc_mode gc_mode)
 {
diff --git a/test/unit/swim_test_utils.h b/test/unit/swim_test_utils.h
index ac86b6f72..b413bb8ce 100644
--- a/test/unit/swim_test_utils.h
+++ b/test/unit/swim_test_utils.h
@@ -64,6 +64,13 @@ void
 swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
 		       enum crypto_mode mode, const char *key, int key_size);
 
+/**
+ * Delete codec for each instance in
+ * @a cluster.
+ */
+void
+swim_cluster_delete_codec(struct swim_cluster *cluster);
+
 /**
  * Change number of unacknowledged pings to delete a dead member
  * of all the instances in the cluster.
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v1] asan: leak unit/swim.test:swim_test_encryption

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

I force pushed my review fixes on top of the branch and paste
them below. If you agree, and if it works (I wasn't able to reproduce
the fail locally), then squash, please.

====================
diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
index 53f118609..396bd7c45 100644
--- a/src/lib/swim/swim.c
+++ b/src/lib/swim/swim.c
@@ -2114,12 +2114,6 @@ swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
 					key, key_size);
 }
 
-int
-swim_delete_codec(struct swim *swim)
-{
-	return swim_scheduler_delete_codec(&swim->scheduler);
-}
-
 bool
 swim_is_configured(const struct swim *swim)
 {
diff --git a/src/lib/swim/swim.h b/src/lib/swim/swim.h
index e684205c4..4565eb976 100644
--- a/src/lib/swim/swim.h
+++ b/src/lib/swim/swim.h
@@ -124,15 +124,6 @@ int
 swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
 	       const char *key, int key_size);
 
-
-/**
- * Delete SWIM codec used to encrypt/decrypt messages.
- * @param swim SWIM instance to set codec for.
- */
-int
-swim_delete_codec(struct swim *swim);
-
-
 /**
  * Stop listening and broadcasting messages, cleanup all internal
  * structures, free memory. The function yields. Actual deletion
diff --git a/src/lib/swim/swim_io.c b/src/lib/swim/swim_io.c
index f7107d39e..c8558c43e 100644
--- a/src/lib/swim/swim_io.c
+++ b/src/lib/swim/swim_io.c
@@ -402,6 +402,8 @@ swim_scheduler_destroy(struct swim_scheduler *scheduler)
 	swim_transport_destroy(&scheduler->transport);
 	swim_ev_io_stop(swim_loop(), &scheduler->output);
 	swim_scheduler_stop_input(scheduler);
+	if (scheduler->codec != NULL)
+		crypto_codec_delete(scheduler->codec);
 	assert(scheduler_count > 0);
 	if (--scheduler_count == 0)
 		swim_task_pool_destroy();
@@ -708,14 +710,6 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
 	return 0;
 }
 
-int
-swim_scheduler_delete_codec(struct swim_scheduler *scheduler)
-{
-	if (scheduler->codec != NULL)
-		crypto_codec_delete(scheduler->codec);
-	return 0;
-}
-
 const char *
 swim_inaddr_str(const struct sockaddr_in *addr)
 {
diff --git a/src/lib/swim/swim_io.h b/src/lib/swim/swim_io.h
index 72c6c030d..bf5a1389f 100644
--- a/src/lib/swim/swim_io.h
+++ b/src/lib/swim/swim_io.h
@@ -204,10 +204,6 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
 			 enum crypto_algo algo, enum crypto_mode mode,
 			 const char *key, int key_size);
 
-/** Delete the codec used to encrypt/decrypt messages. */
-int
-swim_scheduler_delete_codec(struct swim_scheduler *scheduler);
-
 /** Stop accepting new packets from the network. */
 void
 swim_scheduler_stop_input(struct swim_scheduler *scheduler);
diff --git a/test/unit/swim.c b/test/unit/swim.c
index bc19f8a10..bb12baf8d 100644
--- a/test/unit/swim.c
+++ b/test/unit/swim.c
@@ -770,7 +770,6 @@ swim_test_encryption(void)
 
 	is(swim_cluster_wait_fullmesh(cluster, 2), 0,
 	   "cluster works with encryption");
-	swim_cluster_delete_codec(cluster);
 	swim_cluster_delete(cluster);
 	/*
 	 * Test that the instances can not interact with different
@@ -1141,4 +1140,4 @@ main()
 {
 	swim_run_test("swim.txt", main_f);
 	return test_result;
-}
+}
\ No newline at end of file
diff --git a/test/unit/swim_test_utils.c b/test/unit/swim_test_utils.c
index c527af55c..9dbd28a9f 100644
--- a/test/unit/swim_test_utils.c
+++ b/test/unit/swim_test_utils.c
@@ -237,14 +237,6 @@ swim_cluster_new(int size)
 	}								\
 })
 
-#define swim_cluster_set_cfg_noargs(cluster, func) ({				\
-	for (int i = 0; i < cluster->size; ++i) {			\
-		int rc = func(cluster->node[i].swim);	\
-		assert(rc == 0);					\
-		(void) rc;						\
-	}								\
-})
-
 void
 swim_cluster_set_ack_timeout(struct swim_cluster *cluster, double ack_timeout)
 {
@@ -260,12 +252,6 @@ swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
 			     key, key_size);
 }
 
-void
-swim_cluster_delete_codec(struct swim_cluster *cluster)
-{
-	swim_cluster_set_cfg_noargs(cluster, swim_delete_codec);
-}
-
 void
 swim_cluster_set_gc(struct swim_cluster *cluster, enum swim_gc_mode gc_mode)
 {
diff --git a/test/unit/swim_test_utils.h b/test/unit/swim_test_utils.h
index b413bb8ce..ac86b6f72 100644
--- a/test/unit/swim_test_utils.h
+++ b/test/unit/swim_test_utils.h
@@ -64,13 +64,6 @@ void
 swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
 		       enum crypto_mode mode, const char *key, int key_size);
 
-/**
- * Delete codec for each instance in
- * @a cluster.
- */
-void
-swim_cluster_delete_codec(struct swim_cluster *cluster);
-
 /**
  * Change number of unacknowledged pings to delete a dead member
  * of all the instances in the cluster.

Re: [Tarantool-patches] [PATCH v1] asan: leak unit/swim.test:swim_test_encryption

[Alexander V. Tikhonov]

Hi Vlad, thanks for the review and suggested patch. It is the same as
I started from, but just was afraid to suggest it being too easy, so
sure I already checked it and again rechecked it and agree with it.
Your patch completely LGTM to me, thanks.

On Wed, Sep 09, 2020 at 11:06:43PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> I force pushed my review fixes on top of the branch and paste
> them below. If you agree, and if it works (I wasn't able to reproduce
> the fail locally), then squash, please.
> 
> ====================
> diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
> index 53f118609..396bd7c45 100644
> --- a/src/lib/swim/swim.c
> +++ b/src/lib/swim/swim.c
> @@ -2114,12 +2114,6 @@ swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
>  					key, key_size);
>  }
>  
> -int
> -swim_delete_codec(struct swim *swim)
> -{
> -	return swim_scheduler_delete_codec(&swim->scheduler);
> -}
> -
>  bool
>  swim_is_configured(const struct swim *swim)
>  {
> diff --git a/src/lib/swim/swim.h b/src/lib/swim/swim.h
> index e684205c4..4565eb976 100644
> --- a/src/lib/swim/swim.h
> +++ b/src/lib/swim/swim.h
> @@ -124,15 +124,6 @@ int
>  swim_set_codec(struct swim *swim, enum crypto_algo algo, enum crypto_mode mode,
>  	       const char *key, int key_size);
>  
> -
> -/**
> - * Delete SWIM codec used to encrypt/decrypt messages.
> - * @param swim SWIM instance to set codec for.
> - */
> -int
> -swim_delete_codec(struct swim *swim);
> -
> -
>  /**
>   * Stop listening and broadcasting messages, cleanup all internal
>   * structures, free memory. The function yields. Actual deletion
> diff --git a/src/lib/swim/swim_io.c b/src/lib/swim/swim_io.c
> index f7107d39e..c8558c43e 100644
> --- a/src/lib/swim/swim_io.c
> +++ b/src/lib/swim/swim_io.c
> @@ -402,6 +402,8 @@ swim_scheduler_destroy(struct swim_scheduler *scheduler)
>  	swim_transport_destroy(&scheduler->transport);
>  	swim_ev_io_stop(swim_loop(), &scheduler->output);
>  	swim_scheduler_stop_input(scheduler);
> +	if (scheduler->codec != NULL)
> +		crypto_codec_delete(scheduler->codec);
>  	assert(scheduler_count > 0);
>  	if (--scheduler_count == 0)
>  		swim_task_pool_destroy();
> @@ -708,14 +710,6 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
>  	return 0;
>  }
>  
> -int
> -swim_scheduler_delete_codec(struct swim_scheduler *scheduler)
> -{
> -	if (scheduler->codec != NULL)
> -		crypto_codec_delete(scheduler->codec);
> -	return 0;
> -}
> -
>  const char *
>  swim_inaddr_str(const struct sockaddr_in *addr)
>  {
> diff --git a/src/lib/swim/swim_io.h b/src/lib/swim/swim_io.h
> index 72c6c030d..bf5a1389f 100644
> --- a/src/lib/swim/swim_io.h
> +++ b/src/lib/swim/swim_io.h
> @@ -204,10 +204,6 @@ swim_scheduler_set_codec(struct swim_scheduler *scheduler,
>  			 enum crypto_algo algo, enum crypto_mode mode,
>  			 const char *key, int key_size);
>  
> -/** Delete the codec used to encrypt/decrypt messages. */
> -int
> -swim_scheduler_delete_codec(struct swim_scheduler *scheduler);
> -
>  /** Stop accepting new packets from the network. */
>  void
>  swim_scheduler_stop_input(struct swim_scheduler *scheduler);
> diff --git a/test/unit/swim.c b/test/unit/swim.c
> index bc19f8a10..bb12baf8d 100644
> --- a/test/unit/swim.c
> +++ b/test/unit/swim.c
> @@ -770,7 +770,6 @@ swim_test_encryption(void)
>  
>  	is(swim_cluster_wait_fullmesh(cluster, 2), 0,
>  	   "cluster works with encryption");
> -	swim_cluster_delete_codec(cluster);
>  	swim_cluster_delete(cluster);
>  	/*
>  	 * Test that the instances can not interact with different
> @@ -1141,4 +1140,4 @@ main()
>  {
>  	swim_run_test("swim.txt", main_f);
>  	return test_result;
> -}
> +}
> \ No newline at end of file
> diff --git a/test/unit/swim_test_utils.c b/test/unit/swim_test_utils.c
> index c527af55c..9dbd28a9f 100644
> --- a/test/unit/swim_test_utils.c
> +++ b/test/unit/swim_test_utils.c
> @@ -237,14 +237,6 @@ swim_cluster_new(int size)
>  	}								\
>  })
>  
> -#define swim_cluster_set_cfg_noargs(cluster, func) ({				\
> -	for (int i = 0; i < cluster->size; ++i) {			\
> -		int rc = func(cluster->node[i].swim);	\
> -		assert(rc == 0);					\
> -		(void) rc;						\
> -	}								\
> -})
> -
>  void
>  swim_cluster_set_ack_timeout(struct swim_cluster *cluster, double ack_timeout)
>  {
> @@ -260,12 +252,6 @@ swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
>  			     key, key_size);
>  }
>  
> -void
> -swim_cluster_delete_codec(struct swim_cluster *cluster)
> -{
> -	swim_cluster_set_cfg_noargs(cluster, swim_delete_codec);
> -}
> -
>  void
>  swim_cluster_set_gc(struct swim_cluster *cluster, enum swim_gc_mode gc_mode)
>  {
> diff --git a/test/unit/swim_test_utils.h b/test/unit/swim_test_utils.h
> index b413bb8ce..ac86b6f72 100644
> --- a/test/unit/swim_test_utils.h
> +++ b/test/unit/swim_test_utils.h
> @@ -64,13 +64,6 @@ void
>  swim_cluster_set_codec(struct swim_cluster *cluster, enum crypto_algo algo,
>  		       enum crypto_mode mode, const char *key, int key_size);
>  
> -/**
> - * Delete codec for each instance in
> - * @a cluster.
> - */
> -void
> -swim_cluster_delete_codec(struct swim_cluster *cluster);
> -
>  /**
>   * Change number of unacknowledged pings to delete a dead member
>   * of all the instances in the cluster.

Re: [Tarantool-patches] [PATCH v1] asan: leak unit/swim.test:swim_test_encryption

[Kirill Yukhin]

Hello,

On 09 сен 21:08, Alexander V. Tikhonov wrote:
> Found leak issue:
> 
>   [001] +==41031==ERROR: LeakSanitizer: detected memory leaks
>   [001] +
>   [001] +Direct leak of 96 byte(s) in 2 object(s) allocated from:
>   [001] +    #0 0x4d8e53 in __interceptor_malloc (/tnt/test/unit/swim.test+0x4d8e53)
>   [001] +    #1 0x53560f in crypto_codec_new /source/src/lib/crypto/crypto.c:239:51
>   [001] +    #2 0x5299c4 in swim_scheduler_set_codec /source/src/lib/swim/swim_io.c:700:30
>   [001] +    #3 0x511fe6 in swim_cluster_set_codec /source/test/unit/swim_test_utils.c:251:2
>   [001] +    #4 0x50b3ae in swim_test_encryption /source/test/unit/swim.c:767:2
>   [001] +    #5 0x50b3ae in main_f /source/test/unit/swim.c:1123
>   [001] +    #6 0x544a3b in fiber_loop /source/src/lib/core/fiber.c:869:18
>   [001] +    #7 0x5a13d0 in coro_init /source/third_party/coro/coro.c:110:3
>   [001] +
>   [001] +SUMMARY: AddressSanitizer: 96 byte(s) leaked in 2 allocation(s).
> 
> Prepared minimal issue reproducer:
> 
>   static void
>   swim_test_encryption(void)
>   {
>           swim_start_test(3);
>           struct swim_cluster *cluster = swim_cluster_new(2);
>           swim_cluster_set_codec(cluster, CRYPTO_ALGO_AES128, CRYPTO_MODE_CBC,
>                                  "1234567812345678", CRYPTO_AES128_KEY_SIZE);
>           swim_cluster_delete(cluster);
>           swim_finish_test();
>   }
> 
> Found that memory allocated for codec creation at crypto_codec_new()
> using swim_cluster_set_codec(). But there was no any memory free for
> it. Decided to integrate at test utilities swim_cluster_delete_codec()
> function for it available for tests.
> 
> After this fix removed susspencion on memory leak for unit/swin.test.
> 
> Closes #5283
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5283-asan-swim
> Issue: https://github.com/tarantool/tarantool/issues/5283

I've checked your patch into 2.4, 2.5 and master.

--
Regards, Kirill Yukhin