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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel