[Spice-devel] [PATCH spice-server 2/2] memslot: Remove error parameter from memslot_get_virt

Christophe Fergeau cfergeau at redhat.com
Tue Jun 26 13:50:17 UTC 2018


On Tue, Jun 19, 2018 at 11:05:26AM +0100, Frediano Ziglio wrote:
> Pointers to memory allocated in user space are never NULL.
> The only exception can be if you explicitly map memory at zero.
> There is however no reasons for such requirement and this practise
> was also removed from Linux due to security reasons.
> This API looks copied from a kernel environment where valid virtual
> addresses can be NULL.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/memslot.c        | 12 ++----
>  server/memslot.h        |  2 +-
>  server/red-parse-qxl.c  | 92 +++++++++++++++++------------------------
>  server/red-record-qxl.c | 74 +++++++++------------------------
>  server/red-worker.c     | 14 +++----
>  5 files changed, 66 insertions(+), 128 deletions(-)
> 
> diff --git a/server/memslot.c b/server/memslot.c
> index a87a717b..ede77e7a 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -86,11 +86,10 @@ unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
>  }
>  
>  /*
> - * return virtual address if successful, which may be 0.
> - * returns 0 and sets error to 1 if an error condition occurs.
> + * returns NULL on failure.
>   */
> -void* memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
> -                       int group_id, int *error)
> +void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
> +                       int group_id)
>  {
>      int slot_id;
>      int generation;
> @@ -98,10 +97,8 @@ void* memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
>  
>      MemSlot *slot;
>  
> -    *error = 0;
>      if (group_id > info->num_memslots_groups) {
>          spice_critical("group_id too big");
> -        *error = 1;
>          return NULL;
>      }
>  
> @@ -109,7 +106,6 @@ void* memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
>      if (slot_id > info->num_memslots) {
>          print_memslots(info);
>          spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr);
> -        *error = 1;
>          return NULL;
>      }
>  
> @@ -120,7 +116,6 @@ void* memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
>          print_memslots(info);
>          spice_critical("address generation is not valid, group_id %d, slot_id %d, gen %d, slot_gen %d\n",
>                group_id, slot_id, generation, slot->generation);
> -        *error = 1;
>          return NULL;
>      }
>  
> @@ -128,7 +123,6 @@ void* memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
>      h_virt += slot->address_delta;
>  
>      if (!memslot_validate_virt(info, h_virt, slot_id, add_size, group_id)) {
> -        *error = 1;
>          return NULL;
>      }
>  
> diff --git a/server/memslot.h b/server/memslot.h
> index d8d67d55..00728c4b 100644
> --- a/server/memslot.h
> +++ b/server/memslot.h
> @@ -59,7 +59,7 @@ unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
>                                      unsigned long virt, int slot_id,
>                                      uint32_t group_id);
>  void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
> -                       int group_id, int *error);
> +                       int group_id);
>  
>  void memslot_info_init(RedMemSlotInfo *info,
>                         uint32_t num_groups, uint32_t num_slots,
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index d28c935f..b0af1496 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -120,7 +120,6 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
>      RedDataChunk *red_prev;
>      uint64_t data_size = 0;
>      uint32_t chunk_data_size;
> -    int error;
>      QXLPHYSICAL next_chunk;
>      unsigned num_chunks = 0;
>  
> @@ -143,10 +142,10 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
>          }
>  
>          memslot_id = memslot_get_id(slots, next_chunk);
> -        qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, sizeof(*qxl),
> -                                               group_id, &error);
> -        if (error)
> +        qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, sizeof(*qxl), group_id);
> +        if (!qxl) {

Strong preference for if (qxl == NULL) here and below

>              goto error;
> +        }
>  
>          /* do not waste space for empty chunks.
>           * This could be just a driver issue or an attempt
> @@ -194,11 +193,10 @@ static size_t red_get_data_chunks(RedMemSlotInfo *slots, int group_id,
>                                    RedDataChunk *red, QXLPHYSICAL addr)
>  {
>      QXLDataChunk *qxl;
> -    int error;
>      int memslot_id = memslot_get_id(slots, addr);
>  
> -    qxl = (QXLDataChunk *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLDataChunk *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return INVALID_SIZE;
>      }
>      return red_get_data_chunks_ptr(slots, group_id, memslot_id, red, qxl);
> @@ -251,10 +249,9 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
>      int n_segments;
>      int i;
>      uint32_t count;
> -    int error;
>  
> -    qxl = (QXLPath *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLPath *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return NULL;
>      }
>      size = red_get_data_chunks_ptr(slots, group_id,
> @@ -329,11 +326,10 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
>      bool free_data;
>      size_t size;
>      int i;
> -    int error;
>      uint32_t num_rects;
>  
> -    qxl = (QXLClipRects *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLClipRects *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return NULL;
>      }
>      size = red_get_data_chunks_ptr(slots, group_id,
> @@ -370,11 +366,10 @@ static SpiceChunks *red_get_image_data_flat(RedMemSlotInfo *slots, int group_id,
>                                              QXLPHYSICAL addr, size_t size)
>  {
>      SpiceChunks *data;
> -    int error;
>      void *bitmap_virt;
>  
> -    bitmap_virt = memslot_get_virt(slots, addr, size, group_id, &error);
> -    if (error) {
> +    bitmap_virt = memslot_get_virt(slots, addr, size, group_id);
> +    if (!bitmap_virt) {
>          return 0;

You could change the '0' to NULL while at it.

>      }
>  
> @@ -460,15 +455,14 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
>      SpicePalette *rp = NULL;
>      uint64_t bitmap_size, size;
>      uint8_t qxl_flags;
> -    int error;
>      QXLPHYSICAL palette;
>  
>      if (addr == 0) {
>          return NULL;
>      }
>  
> -    qxl = (QXLImage *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLImage *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return NULL;
>      }
>      red = g_new0(SpiceImage, 1);
> @@ -511,8 +505,8 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
>              QXLPalette *qp;
>              int i, num_ents;
>              qp = (QXLPalette *)memslot_get_virt(slots, palette,
> -                                                sizeof(*qp), group_id, &error);
> -            if (error) {
> +                                                sizeof(*qp), group_id);
> +            if (!qp) {
>                  goto error;
>              }
>              num_ents = qp->num_ents;
> @@ -759,14 +753,13 @@ static bool get_transform(RedMemSlotInfo *slots,
>                            SpiceTransform *dst_transform)
>  {
>      const uint32_t *t = NULL;
> -    int error;
>  
>      if (qxl_transform == 0)
>          return false;
>  
> -    t = (uint32_t *)memslot_get_virt(slots, qxl_transform, sizeof(*dst_transform), group_id, &error);
> +    t = (uint32_t *)memslot_get_virt(slots, qxl_transform, sizeof(*dst_transform), group_id);
>  
> -    if (!t || error)
> +    if (!t)
>          return false;
>  
>      memcpy(dst_transform, t, sizeof(*dst_transform));
> @@ -824,8 +817,6 @@ static void red_put_rop3(SpiceRop3 *red)
>  static bool red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
>                                 SpiceStroke *red, QXLStroke *qxl, uint32_t flags)
>  {
> -    int error;
> -
>      red->path = red_get_path(slots, group_id, qxl->path);
>      if (!red->path) {
>          return false;
> @@ -840,8 +831,8 @@ static bool red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
>          red->attr.style_nseg  = style_nseg;
>          spice_assert(qxl->attr.style);
>          buf = (uint8_t *)memslot_get_virt(slots, qxl->attr.style,
> -                                          style_nseg * sizeof(QXLFIXED), group_id, &error);
> -        if (error) {
> +                                          style_nseg * sizeof(QXLFIXED), group_id);
> +        if (!buf) {
>              return false;
>          }
>          memcpy(red->attr.style, buf, style_nseg * sizeof(QXLFIXED));
> @@ -878,11 +869,10 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
>      int glyphs, i;
>      /* use unsigned to prevent integer overflow in multiplication below */
>      unsigned int bpp = 0;
> -    int error;
>      uint16_t qxl_flags, qxl_length;
>  
> -    qxl = (QXLString *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLString *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return NULL;
>      }
>      chunk_size = red_get_data_chunks_ptr(slots, group_id,
> @@ -1016,10 +1006,9 @@ static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
>  {
>      QXLDrawable *qxl;
>      int i;
> -    int error = 0;
>  
> -    qxl = (QXLDrawable *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLDrawable *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info     = &qxl->release_info;
> @@ -1096,10 +1085,9 @@ static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
>                                      RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
>  {
>      QXLCompatDrawable *qxl;
> -    int error;
>  
> -    qxl = (QXLCompatDrawable *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLCompatDrawable *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info     = &qxl->release_info;
> @@ -1241,10 +1229,9 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
>                          RedUpdateCmd *red, QXLPHYSICAL addr)
>  {
>      QXLUpdateCmd *qxl;
> -    int error;
>  
> -    qxl = (QXLUpdateCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLUpdateCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info     = &qxl->release_info;
> @@ -1266,7 +1253,6 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,
>                       RedMessage *red, QXLPHYSICAL addr)
>  {
>      QXLMessage *qxl;
> -    int error;
>      int memslot_id;
>      unsigned long len;
>      uint8_t *end;
> @@ -1277,8 +1263,8 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,
>       *   luckily this is for debug logging only,
>       *   so we can just ignore it by default.
>       */
> -    qxl = (QXLMessage *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLMessage *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info      = &qxl->release_info;
> @@ -1351,11 +1337,9 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
>  {
>      QXLSurfaceCmd *qxl;
>      uint64_t size;
> -    int error;
>  
> -    qxl = (QXLSurfaceCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id,
> -                                            &error);
> -    if (error) {
> +    qxl = (QXLSurfaceCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info      = &qxl->release_info;
> @@ -1379,8 +1363,8 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
>  
>          size = red->u.surface_create.height * abs(red->u.surface_create.stride);
>          red->u.surface_create.data =
> -            (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data, size, group_id, &error);
> -        if (error) {
> +            (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data, size, group_id);
> +        if (!red->u.surface_create.data) {
>              return false;
>          }
>          break;
> @@ -1401,10 +1385,9 @@ static bool red_get_cursor(RedMemSlotInfo *slots, int group_id,
>      size_t size;
>      uint8_t *data;
>      bool free_data;
> -    int error;
>  
> -    qxl = (QXLCursor *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLCursor *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>  
> @@ -1443,10 +1426,9 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
>                          RedCursorCmd *red, QXLPHYSICAL addr)
>  {
>      QXLCursorCmd *qxl;
> -    int error;
>  
> -    qxl = (QXLCursorCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);
> -    if (error) {
> +    qxl = (QXLCursorCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id);
> +    if (!qxl) {
>          return false;
>      }
>      red->release_info_ext.info      = &qxl->release_info;
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 5f6b7aeb..0ae72e9b 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -42,10 +42,8 @@ static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
>  {
>      uint8_t *hex;
>      int i;
> -    int error;
>  
> -    hex = (uint8_t*)memslot_get_virt(slots, addr, bytes, group_id,
> -                                     &error);
> +    hex = (uint8_t*)memslot_get_virt(slots, addr, bytes, group_id);
>      for (i = 0; i < bytes; i++) {
>          if (0 == i % 16) {
>              fprintf(stderr, "%lx: ", addr+i);
> @@ -144,12 +142,10 @@ static size_t red_record_data_chunks_ptr(FILE *fd, const char *prefix,
>      size_t data_size = qxl->data_size;
>      int count_chunks = 0;
>      QXLDataChunk *cur = qxl;
> -    int error;
>  
>      while (cur->next_chunk) {
>          cur =
> -            (QXLDataChunk*)memslot_get_virt(slots, cur->next_chunk, sizeof(*cur), group_id,
> -                                            &error);
> +            (QXLDataChunk*)memslot_get_virt(slots, cur->next_chunk, sizeof(*cur), group_id);
>          data_size += cur->data_size;
>          count_chunks++;
>      }
> @@ -159,8 +155,7 @@ static size_t red_record_data_chunks_ptr(FILE *fd, const char *prefix,
>  
>      while (qxl->next_chunk) {
>          memslot_id = memslot_get_id(slots, qxl->next_chunk);
> -        qxl = (QXLDataChunk*)memslot_get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id,
> -                                              &error);
> +        qxl = (QXLDataChunk*)memslot_get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id);
>  

Should we add error checking to red-record-qxl.c? Or are we fine with no
error checks as 1) this is mostly for debug, and not active until the
appropriate env var is set 2) this comes after red_qxl_get_command()
which will already have done the checking for us?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180626/a0ad35c7/attachment.sig>


More information about the Spice-devel mailing list