Add judgement in compress_threads_save_cleanup() to check whether the
static CompressParam *comp_param has been allocated. If not, just
return; or else segmentation fault will occur when using the NULL
comp_param's parameters.  One test case can reproduce this is: set
the compression on and migrate to a wrong nonexistent host IP address.

Our current code does not judge before handling comp_param[idx]'s quit
and cond that whether they have been initialized. If not initialized,
"qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will
occur. Fix this by squashing the terminate_compression_threads() into
compress_threads_save_cleanup() and employing the existing judgement
condition.  One test case can reproduce this error is: set the
compression on and fail to fully setup the default eight compression
thread in compress_threads_save_setup().

Signed-off-by: Fei Li <fli@suse.com>
---
 migration/ram.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f6fd8e5e09..050d535068 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -420,28 +420,14 @@ static void *do_data_compress(void *opaque)
     return NULL;
 }
 
-static inline void terminate_compression_threads(void)
-{
-    int idx, thread_count;
-
-    thread_count = migrate_compress_threads();
-
-    for (idx = 0; idx < thread_count; idx++) {
-        qemu_mutex_lock(&comp_param[idx].mutex);
-        comp_param[idx].quit = true;
-        qemu_cond_signal(&comp_param[idx].cond);
-        qemu_mutex_unlock(&comp_param[idx].mutex);
-    }
-}
-
 static void compress_threads_save_cleanup(void)
 {
     int i, thread_count;
 
-    if (!migrate_use_compression()) {
+    if (!migrate_use_compression() || !comp_param) {
         return;
     }
-    terminate_compression_threads();
+
     thread_count = migrate_compress_threads();
     for (i = 0; i < thread_count; i++) {
         /*
@@ -451,6 +437,12 @@ static void compress_threads_save_cleanup(void)
         if (!comp_param[i].file) {
             break;
         }
+
+        qemu_mutex_lock(&comp_param[i].mutex);
+        comp_param[i].quit = true;
+        qemu_cond_signal(&comp_param[i].cond);
+        qemu_mutex_unlock(&comp_param[i].mutex);
+
         qemu_thread_join(compress_threads + i);
         qemu_mutex_destroy(&comp_param[i].mutex);
         qemu_cond_destroy(&comp_param[i].cond);
-- 
2.13.7

On Tue, Sep 25, 2018 at 05:14:40PM +0800, Fei Li wrote:
> Add judgement in compress_threads_save_cleanup() to check whether the
> static CompressParam *comp_param has been allocated. If not, just
> return; or else segmentation fault will occur when using the NULL
> comp_param's parameters.  One test case can reproduce this is: set
> the compression on and migrate to a wrong nonexistent host IP address.
> 
> Our current code does not judge before handling comp_param[idx]'s quit
> and cond that whether they have been initialized. If not initialized,
> "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will
> occur. Fix this by squashing the terminate_compression_threads() into
> compress_threads_save_cleanup() and employing the existing judgement
> condition.  One test case can reproduce this error is: set the
> compression on and fail to fully setup the default eight compression
> thread in compress_threads_save_setup().
> 
> Signed-off-by: Fei Li <fli@suse.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Regards,

-- 
Peter Xu

* Fei Li (fli@suse.com) wrote:
> Add judgement in compress_threads_save_cleanup() to check whether the
> static CompressParam *comp_param has been allocated. If not, just
> return; or else segmentation fault will occur when using the NULL
> comp_param's parameters.  One test case can reproduce this is: set
> the compression on and migrate to a wrong nonexistent host IP address.
> 
> Our current code does not judge before handling comp_param[idx]'s quit
> and cond that whether they have been initialized. If not initialized,
> "qemu_mutex_lock_impl: Assertion `mutex->initialized' failed." will
> occur. Fix this by squashing the terminate_compression_threads() into
> compress_threads_save_cleanup() and employing the existing judgement
> condition.  One test case can reproduce this error is: set the
> compression on and fail to fully setup the default eight compression
> thread in compress_threads_save_setup().
> 
> Signed-off-by: Fei Li <fli@suse.com>

Queued.

> ---
>  migration/ram.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f6fd8e5e09..050d535068 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -420,28 +420,14 @@ static void *do_data_compress(void *opaque)
>      return NULL;
>  }
>  
> -static inline void terminate_compression_threads(void)
> -{
> -    int idx, thread_count;
> -
> -    thread_count = migrate_compress_threads();
> -
> -    for (idx = 0; idx < thread_count; idx++) {
> -        qemu_mutex_lock(&comp_param[idx].mutex);
> -        comp_param[idx].quit = true;
> -        qemu_cond_signal(&comp_param[idx].cond);
> -        qemu_mutex_unlock(&comp_param[idx].mutex);
> -    }
> -}
> -
>  static void compress_threads_save_cleanup(void)
>  {
>      int i, thread_count;
>  
> -    if (!migrate_use_compression()) {
> +    if (!migrate_use_compression() || !comp_param) {
>          return;
>      }
> -    terminate_compression_threads();
> +
>      thread_count = migrate_compress_threads();
>      for (i = 0; i < thread_count; i++) {
>          /*
> @@ -451,6 +437,12 @@ static void compress_threads_save_cleanup(void)
>          if (!comp_param[i].file) {
>              break;
>          }
> +
> +        qemu_mutex_lock(&comp_param[i].mutex);
> +        comp_param[i].quit = true;
> +        qemu_cond_signal(&comp_param[i].cond);
> +        qemu_mutex_unlock(&comp_param[i].mutex);
> +
>          qemu_thread_join(compress_threads + i);
>          qemu_mutex_destroy(&comp_param[i].mutex);
>          qemu_cond_destroy(&comp_param[i].cond);
> -- 
> 2.13.7
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK