[Spice-devel] [spice-gtk v1 10/10] file-transfer: fix leak on _task_get_filename

[Victor Toso]

Hi,

On Sat, Jul 30, 2016 at 12:26:31AM +0200, Victor Toso wrote:
> This fixes the leak of filename. The documentation says that this is
> transfer none so Spicy and Virt-Viewer are not trying to free this
> string.
> 
> The other option would be changing the documentation to transfer full
> and fix the applications.
> 
> This patch breaks API by using const* to return value and ABI as
> client application that was free'ing will need to fix it.
> 
> While on it, we use a lot of g_file_get_basename() in the code so I
> kept the filename saved as it should not change during the file
> transfer.
> ---
>  src/spice-file-transfer-task.c | 20 +++++++++-----------
>  src/spice-file-transfer-task.h |  2 +-
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> index fb48b9b..70571f0 100644
> --- a/src/spice-file-transfer-task.c
> +++ b/src/spice-file-transfer-task.c
> @@ -45,6 +45,7 @@ struct _SpiceFileTransferTask
>      gboolean                       completed;
>      gboolean                       pending;
>      GFile                          *file;
> +    gchar                          *filename;
>      SpiceMainChannel               *channel;
>      GFileInputStream               *file_stream;
>      GFileCopyFlags                 flags;
> @@ -212,11 +213,9 @@ static void spice_file_transfer_task_read_stream_cb(GObject *source_object,
>          gint64 now = g_get_monotonic_time();
>  
>          if (interval < now - self->last_update) {
> -            gchar *basename = g_file_get_basename(self->file);
>              self->last_update = now;
>              SPICE_DEBUG("read %.2f%% of the file %s",
> -                        100.0 * self->read_bytes / self->file_size, basename);
> -            g_free(basename);
> +                        100.0 * self->read_bytes / self->file_size, self->filename);
>          }
>      }
>  
> @@ -246,16 +245,14 @@ static void spice_file_transfer_task_close_stream_cb(GObject      *object,
>  
>      if (self->error == NULL && spice_util_get_debug()) {
>          gint64 now = g_get_monotonic_time();
> -        gchar *basename = g_file_get_basename(self->file);
>          double seconds = (double) (now - self->start_time) / G_TIME_SPAN_SECOND;
>          gchar *file_size_str = g_format_size(self->file_size);
>          gchar *transfer_speed_str = g_format_size(self->file_size / seconds);
>  
>          g_warn_if_fail(self->read_bytes == self->file_size);
>          SPICE_DEBUG("transferred file %s of %s size in %.1f seconds (%s/s)",
> -                    basename, file_size_str, seconds, transfer_speed_str);
> +                    self->filename, file_size_str, seconds, transfer_speed_str);
>  
> -        g_free(basename);
>          g_free(file_size_str);
>          g_free(transfer_speed_str);
>      }
> @@ -540,9 +537,9 @@ void spice_file_transfer_task_cancel(SpiceFileTransferTask *self)
>   *
>   * Since: 0.31
>   **/
> -char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self)
> +const char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self)
>  {
> -    return g_file_get_basename(self->file);
> +    return self->filename;
>  }
>  
>  /*******************************************************************************
> @@ -608,6 +605,7 @@ spice_file_transfer_task_dispose(GObject *object)
>      g_clear_object(&self->file);
>      g_clear_object(&self->file_stream);
>      g_clear_error(&self->error);
> +    g_clear_pointer(&self->filename, g_free);
>  
>      G_OBJECT_CLASS(spice_file_transfer_task_parent_class)->dispose(object);
>  }
> @@ -626,14 +624,14 @@ static void
>  spice_file_transfer_task_constructed(GObject *object)
>  {
>      SpiceFileTransferTask *self = SPICE_FILE_TRANSFER_TASK(object);
> +    self->filename = g_file_get_basename(self->file);
>  
>      if (spice_util_get_debug()) {
> -        gchar *basename = g_file_get_basename(self->file);
>          self->start_time = g_get_monotonic_time();
>          self->last_update = self->start_time;
>  
> -        SPICE_DEBUG("transfer of file %s has started", basename);
> -        g_free(basename);
> +        SPICE_DEBUG("transfer of file %s has started", self->filename);
> +        g_free(self->filename);

This free above is wrong, we should only free it on _finalize().
Fixing it..

>      }
>  }
>  
> diff --git a/src/spice-file-transfer-task.h b/src/spice-file-transfer-task.h
> index 4f179fb..43d1d86 100644
> --- a/src/spice-file-transfer-task.h
> +++ b/src/spice-file-transfer-task.h
> @@ -41,7 +41,7 @@ typedef struct _SpiceFileTransferTaskClass SpiceFileTransferTaskClass;
>  
>  GType spice_file_transfer_task_get_type(void) G_GNUC_CONST;
>  
> -char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self);
> +const char* spice_file_transfer_task_get_filename(SpiceFileTransferTask *self);
>  void spice_file_transfer_task_cancel(SpiceFileTransferTask *self);
>  double spice_file_transfer_task_get_progress(SpiceFileTransferTask *self);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel