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

[Uri Lublin]

On 10/17/2016 01:29 PM, Frediano Ziglio wrote:
>>
>> 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.

Yes, your are right.
I'll send a replacement patch that fixes that.

> Beside that is not clear what the change could cause to the
> upper layer.

Upper layer should already handle a case where these functions
return NULL.

> I think the actual logic is supposing that dictionary creation (like
> memory allocation) is not failing.

Since it depends on a value that comes from the client that
assumption should not be made. Alternatively we
can specifically check the number early

Thanks,
     Uri.