[Spice-devel] [spice-server PATCH 2/8] image_encoders: check shared_dict before accessing it

[Frediano Ziglio]

> 
> In both image_encoders_restore_glz_dictionary() and
> image_encoders_get_glz_dictionary() shared-dict may
> be NULL if size is too large, and the server gets
> size from the network.
> 
> Both functions end up calling glz_enc_dictionary_create()
> that calls glz_dictionary_window_create() where size is
> checked.
> 
> Found by coverity.
> 
> Signed-off-by: Uri Lublin <uril at redhat.com>
> ---
>  server/image-encoders.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/server/image-encoders.c b/server/image-encoders.c
> index 39aca6c..9dfabd6 100644
> --- a/server/image-encoders.c
> +++ b/server/image-encoders.c
> @@ -746,12 +746,13 @@ gboolean
> image_encoders_get_glz_dictionary(ImageEncoders *enc,
>          shared_dict->refs++;
>      } else {
>          shared_dict = create_glz_dictionary(enc, client, id, window_size);
> +        spice_return_val_if_fail(shared_dict != NULL, FALSE);
>          glz_dictionary_list = g_list_prepend(glz_dictionary_list,
>          shared_dict);
>      }
>  
>      pthread_mutex_unlock(&glz_dictionary_list_lock);
>      enc->glz_dict = shared_dict;
> -    return shared_dict != NULL;
> +    return TRUE;
>  }
>  
>  static GlzSharedDictionary *restore_glz_dictionary(ImageEncoders *enc,
> @@ -782,12 +783,13 @@ gboolean
> image_encoders_restore_glz_dictionary(ImageEncoders *enc,
>          shared_dict->refs++;
>      } else {
>          shared_dict = restore_glz_dictionary(enc, client, id, restore_data);
> +        spice_return_val_if_fail(shared_dict != NULL, FALSE);
>          glz_dictionary_list = g_list_prepend(glz_dictionary_list,
>          shared_dict);
>      }
>  
>      pthread_mutex_unlock(&glz_dictionary_list_lock);
>      enc->glz_dict = shared_dict;
> -    return shared_dict != NULL;
> +    return TRUE;
>  }
>  
>  gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id)

Note that you are creating dead locks.
Beside that is not clear what the change could cause to the
upper layer.
I think the actual logic is supposing that dictionary creation (like
memory allocation) is not failing.

Frediano