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

[Alon Levy]

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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel