[Spice-devel] [PATCH spice-server] cursor-channel: Remove obsolete size check

[Frediano Ziglio]

> 
> Hi,
> 
> On Mon, May 22, 2017 at 11:41:02AM +0100, Frediano Ziglio wrote:
> > In the past CursorItem structure was stored in
> > RedCursorCmd::device_data field so the check was there to check if the
> > structure fit into that field.
> > Since 2ba69f9f8819daaa3d166c4c1c7e03b121b88a95
> 
> A lit bit hard to follow, this one
> 

Yes, the mentioned patch is quite big and the topic is not much related
to this. However you can see that "device_data" access was removed.

> > ("libspice: add surface 0 support") the structure is no more stored in
> > the field so there's no reason for this check causing only confusion.
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/cursor-channel.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 6aea64a..a876113 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -34,8 +34,6 @@ typedef struct CursorItem {
> >      RedCursorCmd *red_cursor;
> >  } CursorItem;
> >
> > -G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
> > -
> 
> I don't see any other use for QXL_CURSUR_DEVICE_DATA_SIZE in my local
> workspace besides:
> 
>   spice-protocol/spice/qxl_dev.h:335:
>   #define QXL_CURSUR_DEVICE_DATA_SIZE 128
>   spice-protocol/spice/qxl_dev.h:352:
>   uint8_t device_data[QXL_CURSUR_DEVICE_DATA_SIZE]; //todo: dynamic size from
>   rom
> 
> Is that useless now?
> 

Maybe but is not so easy to remove. Now, did some grep too and basically
nobody uses device_data field. Is not surprising as was used to store
spice-server data in qxl card memory... once spice-server usage was
gone the fields stopped to be used.

However this now is an ABI and the size of QXLCursorCmd is part of
the ABI. Although this is the last field and QXLCursorCmd is not
included in any array/structures (but just pointed from the cursor
ring) this can cause problems. Let say for instance that the field is
removed. The QXL drivers will allocate less bytes and pass less bytes.
Now if the spice-server you are using is using the old length it will
check for memory bounds using the old (bigger) size. If the cursor
commands was allocated at the end of QXL memory region (quite unlikely
but not impossible) the red-parse-qxl.c code will detect an error and
discard the command. Not sure if this is the only issue of removing
the field but I think that this change is not strongly related to
this one and should be addresses in a follow up.

> (hmm, checking qxl-wddm-dod)
> [0] https://gitlab.com/spice/qxl-wddm-dod
> 
> Okay, here is also not used and header is actually duplicated (shouldn't
> we use the header from spice-protocol here? see:
> 
>   $ qxl-wddm-dod (master 1a81d6d3) $ grepi "qxl_dev" *
>   qxldod/include/qxl_dev.h:32:#ifndef _H_QXL_DEV
>   qxldod/include/qxl_dev.h:33:#define _H_QXL_DEV
>   qxldod/include/qxl_dev.h:65:#define QXL_DEVICE_ID_STABLE 0x0100
>   qxldod/include/qxl_dev.h:74:#define QXL_DEVICE_ID_DEVEL 0x01ff
>   qxldod/include/qxl_dev.h:808:#endif /* _H_QXL_DEV */
>   qxldod/QxlDod.h:13:#include "qxl_dev.h"
> 
> Anyway, patch looks fine but we might want to do some follow ups.
> 

Yes, surely.

> >  struct CursorChannel
> >  {
> >      CommonGraphicsChannel parent;

Frediano