[Spice-devel] [win32-qxl]Do not allow duplicate IDs in video mode info buffer.

[Christophe Fergeau]

On Wed, Sep 16, 2015 at 03:27:49PM -0400, Sandy Stutsman wrote:
> 
> Modes are returned in a buffer from qemu with IDs in increasing but not
> necessarily consecutive order. (Some modes may be omitted if they don't fit
> into the framebuffer.) Two custom modes are appended to the local buffer to
> support arbitrary resolutions. The first custom id must be set to +1 of the
> last id in the mode buffer, not to the last index +1 of the buffer.
> 
> E.G. these are the last entries in the mode buffer returned from QEMU:
>     Index      Mode id (unfortunately named ModeIndex in the struct)
>     ...
>     124         128
>     125         130
>     126         132
>     127         134 (last mode returned from QEMU)
> 
> Custom Modes:
>     128         135 (Not 128!)
>     129         136
> 
> If the first custom mode id is set to 128 (the number of modes returned
> from QEMU), it will duplicate the id for the mode at index 124. That could
> cause the FindMode function which searches the mode list by mode id
> to return the wrong mode information.
> 
> The custom mode id's are now set to be greater than the last mode id
> returned from QEMU.
> ---
>  xddm/miniport/qxl.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xddm/miniport/qxl.c b/xddm/miniport/qxl.c
> index e0fb87f..22537e2 100644
> --- a/xddm/miniport/qxl.c
> +++ b/xddm/miniport/qxl.c
> @@ -596,6 +596,7 @@ VP_STATUS InitModes(QXLExtension *dev)
>      ULONG n_modes;
>      ULONG i;
>      VP_STATUS error;
> +    ULONG custom_mode_id = 0;
>  
>      PAGED_CODE();
>      DEBUG_PRINT((dev, 0, "%s\n", __FUNCTION__));
> @@ -632,6 +633,11 @@ VP_STATUS InitModes(QXLExtension *dev)
>              return error;
>          }
>      }
> +    /*  Mode ids are increasing in the buffer but may not be consecutive,
> +     *  so set the first custom id to be 1 larger than the last id in the buffer.
> +     *  (ModeIndex isn't really an index, it is an ID).
> +     */
> +    custom_mode_id = modes->modes[modes->n_modes-1].id + 1;
>  
>      /* 2 dummy modes for custom display resolution */
>      /* This is necessary to bypass Windows mode index check, that
> @@ -640,7 +646,7 @@ VP_STATUS InitModes(QXLExtension *dev)
>  
>      for (i = dev->custom_mode; i <= dev->custom_mode + 1; ++i) {
>          memcpy(&modes_info[i], &modes_info[0], sizeof(VIDEO_MODE_INFORMATION));
> -        modes_info[i].ModeIndex = i;
> +        modes_info[i].ModeIndex = custom_mode_id++;
>      }
>  
>      dev->n_modes = n_modes;
> @@ -1002,6 +1008,7 @@ static VP_STATUS SetCustomDisplay(QXLExtension *dev_ext, QXLEscapeSetCustomDispl
>      uint32_t xres = custom_display->xres;
>      uint32_t yres = custom_display->yres;
>      uint32_t bpp = custom_display->bpp;
> +    PVIDEO_MODE_INFORMATION pMode;
>  
>      /* alternate custom mode index */
>      if (dev_ext->custom_mode == (dev_ext->n_modes - 1))
> @@ -1014,11 +1021,11 @@ static VP_STATUS SetCustomDisplay(QXLExtension *dev_ext, QXLEscapeSetCustomDispl
>                      __FUNCTION__, xres, yres, bpp, dev_ext->rom->surface0_area_size));
>          return ERROR_NOT_ENOUGH_MEMORY;
>      }
> -
> -    ret = FillVidModeInfo(&dev_ext->modes[dev_ext->custom_mode],
> +    pMode = &dev_ext->modes[dev_ext->custom_mode];
> +    ret = FillVidModeInfo(pMode,
>                            custom_display->xres, custom_display->yres,
>                            custom_display->bpp,
> -                          dev_ext->custom_mode);
> +                          pMode->ModeIndex);

This looks good to me, ACK.
I'd add a commit on top of that getting rid of
the last argument to FillVidModeInfo as all that function does with this
argument is set pMode->ModeIndex to be equal to its value.

Christophe
-------------- 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/20150917/c281a514/attachment.sig>