[Spice-devel,vdagent-linux] x11: More udscs_read_callback memory ownership issues

Submitted by Christophe Fergeau on Nov. 24, 2016, 3:57 p.m.

Details

Message ID 20161124155742.26539-1-cfergeau@redhat.com
State Accepted
Commit 7324da13b848498ed8c849310addcef497ed4cd5
Headers show
Series "x11: More udscs_read_callback memory ownership issues" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe Fergeau Nov. 24, 2016, 3:57 p.m.
This time it's vdagent_x11_clipboard_data() which is called from
daemon_read_complete() which was missed and which was still freeing data
it no longer owns.

This fixes https://bugs.freedesktop.org/show_bug.cgi?id=98830
---
 src/vdagent/vdagent.c |  2 --
 src/vdagent/x11.c     | 22 +++++++++++-----------
 2 files changed, 11 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
index 085b84a..3d195b1 100644
--- a/src/vdagent/vdagent.c
+++ b/src/vdagent/vdagent.c
@@ -72,8 +72,6 @@  static void daemon_read_complete(struct udscs_connection **connp,
     case VDAGENTD_CLIPBOARD_DATA:
         vdagent_x11_clipboard_data(x11, header->arg1, header->arg2,
                                    data, header->size);
-        /* vdagent_x11_clipboard_data takes ownership of the data (or frees
-           it immediately) */
         break;
     case VDAGENTD_CLIPBOARD_RELEASE:
         vdagent_x11_clipboard_release(x11, header->arg1);
diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
index e88a8ea..4dd1aa8 100644
--- a/src/vdagent/x11.c
+++ b/src/vdagent/x11.c
@@ -1216,7 +1216,6 @@  void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
             SELPRINTF("received clipboard data while still sending"
                       " data from previous request, ignoring");
         }
-        free(data);
         return;
     }
 
@@ -1225,7 +1224,6 @@  void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
             SELPRINTF("received clipboard data without an "
                       "outstanding selection request, ignoring");
         }
-        free(data);
         return;
     }
 
@@ -1244,7 +1242,6 @@  void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
                       type_from_event, type);
         }
         vdagent_x11_send_selection_notify(x11, None, NULL);
-        free(data);
 
         /* Flush output buffers and consume any pending events */
         vdagent_x11_do_read(x11);
@@ -1266,14 +1263,19 @@  void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
                         x11->incr_atom, 32, PropModeReplace,
                         (unsigned char*)&len, 1);
         if (vdagent_x11_restore_error_handler(x11) == 0) {
-            x11->selection_req_data = data;
-            x11->selection_req_data_pos = 0;
-            x11->selection_req_data_size = size;
-            x11->selection_req_atom = prop;
-            vdagent_x11_send_selection_notify(x11, prop, x11->selection_req);
+            /* duplicate data */
+            x11->selection_req_data = malloc(size);
+            if (x11->selection_req_data != NULL) {
+                memcpy(x11->selection_req_data, data, size);
+                x11->selection_req_data_pos = 0;
+                x11->selection_req_data_size = size;
+                x11->selection_req_atom = prop;
+                vdagent_x11_send_selection_notify(x11, prop, x11->selection_req);
+            } else {
+                SELPRINTF("out of memory allocating selection buffer");
+            }
         } else {
             SELPRINTF("clipboard data sent failed, requestor window gone");
-            free(data);
         }
     } else {
         vdagent_x11_set_error_handler(x11, vdagent_x11_ignore_bad_window_handler);
@@ -1284,8 +1286,6 @@  void vdagent_x11_clipboard_data(struct vdagent_x11 *x11, uint8_t selection,
             vdagent_x11_send_selection_notify(x11, prop, NULL);
         else
             SELPRINTF("clipboard data sent failed, requestor window gone");
-
-        free(data);
     }
 
     /* Flush output buffers and consume any pending events */

Comments

On Thu, 2016-11-24 at 16:57 +0100, Christophe Fergeau wrote:
> This time it's vdagent_x11_clipboard_data() which is called from
> daemon_read_complete() which was missed and which was still freeing
> data
> it no longer owns.
> 
> This fixes https://bugs.freedesktop.org/show_bug.cgi?id=98830
Ack

> ---
>  src/vdagent/vdagent.c |  2 --
>  src/vdagent/x11.c     | 22 +++++++++++-----------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 085b84a..3d195b1 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -72,8 +72,6 @@ static void daemon_read_complete(struct
> udscs_connection **connp,
>      case VDAGENTD_CLIPBOARD_DATA:
>          vdagent_x11_clipboard_data(x11, header->arg1, header->arg2,
>                                     data, header->size);
> -        /* vdagent_x11_clipboard_data takes ownership of the data
> (or frees
> -           it immediately) */
>          break;
>      case VDAGENTD_CLIPBOARD_RELEASE:
>          vdagent_x11_clipboard_release(x11, header->arg1);
> diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> index e88a8ea..4dd1aa8 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -1216,7 +1216,6 @@ void vdagent_x11_clipboard_data(struct
> vdagent_x11 *x11, uint8_t selection,
>              SELPRINTF("received clipboard data while still sending"
>                        " data from previous request, ignoring");
>          }
> -        free(data);
>          return;
>      }
>  
> @@ -1225,7 +1224,6 @@ void vdagent_x11_clipboard_data(struct
> vdagent_x11 *x11, uint8_t selection,
>              SELPRINTF("received clipboard data without an "
>                        "outstanding selection request, ignoring");
>          }
> -        free(data);
>          return;
>      }
>  
> @@ -1244,7 +1242,6 @@ void vdagent_x11_clipboard_data(struct
> vdagent_x11 *x11, uint8_t selection,
>                        type_from_event, type);
>          }
>          vdagent_x11_send_selection_notify(x11, None, NULL);
> -        free(data);
>  
>          /* Flush output buffers and consume any pending events */
>          vdagent_x11_do_read(x11);
> @@ -1266,14 +1263,19 @@ void vdagent_x11_clipboard_data(struct
> vdagent_x11 *x11, uint8_t selection,
>                          x11->incr_atom, 32, PropModeReplace,
>                          (unsigned char*)&len, 1);
>          if (vdagent_x11_restore_error_handler(x11) == 0) {
> -            x11->selection_req_data = data;
> -            x11->selection_req_data_pos = 0;
> -            x11->selection_req_data_size = size;
> -            x11->selection_req_atom = prop;
> -            vdagent_x11_send_selection_notify(x11, prop, x11-
> >selection_req);
> +            /* duplicate data */
> +            x11->selection_req_data = malloc(size);
> +            if (x11->selection_req_data != NULL) {
> +                memcpy(x11->selection_req_data, data, size);
> +                x11->selection_req_data_pos = 0;
> +                x11->selection_req_data_size = size;
> +                x11->selection_req_atom = prop;
> +                vdagent_x11_send_selection_notify(x11, prop, x11-
> >selection_req);
> +            } else {
> +                SELPRINTF("out of memory allocating selection
> buffer");
> +            }
>          } else {
>              SELPRINTF("clipboard data sent failed, requestor window
> gone");
> -            free(data);
>          }
>      } else {
>          vdagent_x11_set_error_handler(x11,
> vdagent_x11_ignore_bad_window_handler);
> @@ -1284,8 +1286,6 @@ void vdagent_x11_clipboard_data(struct
> vdagent_x11 *x11, uint8_t selection,
>              vdagent_x11_send_selection_notify(x11, prop, NULL);
>          else
>              SELPRINTF("clipboard data sent failed, requestor window
> gone");
> -
> -        free(data);
>      }
>  
>      /* Flush output buffers and consume any pending events */