[Spice-devel] server/red_dispatcher: fix memset params

Submitted by Uri Lublin on Aug. 2, 2011, 3:05 p.m.

Details

Message ID 1312272311-22789-1-git-send-email-uril@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin Aug. 2, 2011, 3:05 p.m.
Replace wrong memset(ptr, size, 0)
With memset(ptr, 0, size)
---
 server/red_dispatcher.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index 8f4a8a8..f86da81 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -390,7 +390,7 @@  static void red_dispatcher_create_primary_surface_complete(RedDispatcher *dispat
     dispatcher->primary_active = TRUE;

     update_client_mouse_allowed();
-    memset(&dispatcher->surface_create, sizeof(QXLDevSurfaceCreate), 0);
+    memset(&dispatcher->surface_create, 0, sizeof(QXLDevSurfaceCreate));
 }

 static void

Comments

On Tue, Aug 02, 2011 at 11:05:11AM +0300, Uri Lublin wrote:
> Replace wrong memset(ptr, size, 0)
> With memset(ptr, 0, size)

ACK. Thanks for catching this.

> ---
>  server/red_dispatcher.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> index 8f4a8a8..f86da81 100644
> --- a/server/red_dispatcher.c
> +++ b/server/red_dispatcher.c
> @@ -390,7 +390,7 @@ static void red_dispatcher_create_primary_surface_complete(RedDispatcher *dispat
>      dispatcher->primary_active = TRUE;
> 
>      update_client_mouse_allowed();
> -    memset(&dispatcher->surface_create, sizeof(QXLDevSurfaceCreate), 0);
> +    memset(&dispatcher->surface_create, 0, sizeof(QXLDevSurfaceCreate));
>  }
> 
>  static void
> -- 
> 1.7.6
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Hey Uri,

Good catch! How did you find it? valgrind? static analysis? Code
review?

On Tue, Aug 02, 2011 at 11:05:11AM +0300, Uri Lublin wrote:
>      update_client_mouse_allowed();
> -    memset(&dispatcher->surface_create, sizeof(QXLDevSurfaceCreate), 0);
> +    memset(&dispatcher->surface_create, 0, sizeof(QXLDevSurfaceCreate));

It's interesting that the problematic structure is QXLDevSurfaceCreate
because valgrind had issues with it in test_display_streaming too (patch
coming). However, what is interesting is that the QXLDevSurfaceCreate has
4 bytes of padding adding by the compiler on x86_64:

struct QXLDevSurfaceCreate {
        uint32_t                   width;                /*     0     4 */
        uint32_t                   height;               /*     4     4 */
        int32_t                    stride;               /*     8     4 */
        uint32_t                   format;               /*    12     4 */
        uint32_t                   position;             /*    16     4 */
        uint32_t                   mouse_mode;           /*    20     4 */
        uint32_t                   flags;                /*    24     4 */
        uint32_t                   type;                 /*    28     4 */
        uint64_t                   mem;                  /*    32     8 */
        uint32_t                   group_id;             /*    40     4 */

        /* size: 48, cachelines: 1, members: 10 */
        /* padding: 4 */
        /* last cacheline: 48 bytes */
};

As far as I understand, the whole structure is sent on the wire
(for example send_data(dispatcher->channel, surface,
sizeof(QXLDevSurfaceCreate)); in
RedDispatcher::handle_dev_create_primary_surface ), which these 4 bytes
part of the protocol. So I was wondering if we should document somewhere
these 4 bytes of padding? I also haven't checked what size this structure
gets on x86.

Christophe
On Tue, Aug 02, 2011 at 11:55:53AM +0200, Christophe Fergeau wrote:
> Hey Uri,
> 
> Good catch! How did you find it? valgrind? static analysis? Code
> review?
> 
> On Tue, Aug 02, 2011 at 11:05:11AM +0300, Uri Lublin wrote:
> >      update_client_mouse_allowed();
> > -    memset(&dispatcher->surface_create, sizeof(QXLDevSurfaceCreate), 0);
> > +    memset(&dispatcher->surface_create, 0, sizeof(QXLDevSurfaceCreate));
> 
> It's interesting that the problematic structure is QXLDevSurfaceCreate
> because valgrind had issues with it in test_display_streaming too (patch
> coming). However, what is interesting is that the QXLDevSurfaceCreate has
> 4 bytes of padding adding by the compiler on x86_64:
> 
> struct QXLDevSurfaceCreate {
>         uint32_t                   width;                /*     0     4 */
>         uint32_t                   height;               /*     4     4 */
>         int32_t                    stride;               /*     8     4 */
>         uint32_t                   format;               /*    12     4 */
>         uint32_t                   position;             /*    16     4 */
>         uint32_t                   mouse_mode;           /*    20     4 */
>         uint32_t                   flags;                /*    24     4 */
>         uint32_t                   type;                 /*    28     4 */
>         uint64_t                   mem;                  /*    32     8 */
>         uint32_t                   group_id;             /*    40     4 */
> 
>         /* size: 48, cachelines: 1, members: 10 */
>         /* padding: 4 */
>         /* last cacheline: 48 bytes */
> };
> 
> As far as I understand, the whole structure is sent on the wire

Maybe they are sent someplace else, but your example below is hardly on the wire, it's
an internal pipe between two threads, both of the same executable, so presumably all linked
with the same compiler and so with the same padding, so a non issue.

> (for example send_data(dispatcher->channel, surface,
> sizeof(QXLDevSurfaceCreate)); in
> RedDispatcher::handle_dev_create_primary_surface ), which these 4 bytes
> part of the protocol. So I was wondering if we should document somewhere
> these 4 bytes of padding? I also haven't checked what size this structure
> gets on x86.
> 
> Christophe
> 
> 



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel