[Spice-devel] [PATCH 1/4] replay: remove some memory leaks

[Christophe Fergeau]

On Mon, Aug 24, 2015 at 02:20:27PM +0100, Frediano Ziglio wrote:
> Free primary surface memory after creation.
> Free command line options.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red_replay_qxl.c | 1 +
>  server/tests/replay.c   | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/server/red_replay_qxl.c b/server/red_replay_qxl.c
> index a010a58..ca1d584 100644
> --- a/server/red_replay_qxl.c
> +++ b/server/red_replay_qxl.c
> @@ -1074,6 +1074,7 @@ static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay *replay)
>      surface.group_id = 0;
>      surface.mem = (QXLPHYSICAL)mem;
>      worker->create_primary_surface(worker, 0, &surface);
> +    free(mem);

Isn't worker->create_primary_surface() going through
red_dispatcher_create_primary_surface_sync() which will copy 'surface'
including the address stored in surface.mem,  and then use
dispatcher_send_message() to send this to a different thread? If this is
the case, free(mem) right after this call is racy.
Or is the replay code doing something different there?

>  }
>  
>  static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index 01590c0..c35cb52 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -286,12 +286,16 @@ int main(int argc, char **argv)
>          g_printerr("%s\n", g_option_context_get_help(context, TRUE, NULL));
>          exit(1);
>      }
> +    g_option_context_free(context);
> +    context = NULL;
>  
>      if (strncmp(file[0], "-", 1) == 0) {
>          fd = stdin;
>      } else {
>          fd = fopen(file[0], "r");
>      }
> +    g_strfreev(file);
> +    file = NULL;
>      if (fd == NULL) {
>          g_printerr("error opening %s\n", argv[1]);
>          return 1;
> @@ -325,6 +329,8 @@ int main(int argc, char **argv)
>      if (client) {
>          start_client(client, &error);
>          wait = TRUE;
> +        g_free(client);
> +        client = NULL;
>      }

I would not add the xxxx = NULL; lines as this makes things harder to
read for little gain in my opinion, but ACK this way.

Christophe

>  
>      if (!wait) {
> -- 
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150827/461090bc/attachment.sig>