Store SpicePath segment count rather than size

Internally and in the network protocol (for the new version) we now store the actual number of segments rather than the size of the full segments array in bytes. This change consists of multiple changes to handle this: * Make the qxl parser calculate num_segments * Make the canvas stroke code handle the new SpicePath layout. * Fix up is_equal_path in red_worker.c for the new layout * replace multiple calls to spice_marshall_PathSegment with a single spice_marshall_Path call * Make the byte_size() array size handling do the conversion from network size to number of elements when marshalling/demarshalling. * Update the current spice protocol to send the segment count rather than the size * Update the old spice protocol to use the new byte_size functionallity to calculate the size sent and the number of elements recieved
2025-12-28 16:29:56 +00:00 · 2010-06-29 21:42:59 +02:00 · 2010-06-29 21:42:59 +02:00 · a24a8ff72a
commit a24a8ff72a
parent 0f5a6f57b7
11 changed files with 111 additions and 62 deletions
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@ -3077,8 +3077,6 @@ static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
        stroke_fill_spans,
        stroke_fill_rects
    };
-    uint32_t *data_size;
-    uint32_t more;
    SpicePathSeg *seg;
    StrokeLines lines;
    int i;
@ -3182,18 +3180,15 @@ static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
        CANVAS_ERROR("invalid brush type");
    }

-    data_size = (uint32_t*)SPICE_GET_ADDRESS(stroke->path);
-    more = *data_size;
-    seg = (SpicePathSeg*)(data_size + 1);
+    seg = stroke->path->segments;

    stroke_lines_init(&lines);

-    do {
+    for (i = 0; i < stroke->path->num_segments; i++) {
        uint32_t flags = seg->flags;
        SpicePointFix* point = seg->points;
        SpicePointFix* end_point = point + seg->count;
        ASSERT(point < end_point);
-        more -= ((unsigned long)end_point - (unsigned long)seg);
        seg = (SpicePathSeg*)end_point;

        if (flags & SPICE_PATH_BEGIN) {
@ -3223,7 +3218,7 @@ static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
            }
            stroke_lines_draw(&lines, (lineGC *)&gc, dashed);
        }
-    } while (more);
+    }

    stroke_lines_draw(&lines, (lineGC *)&gc, dashed);

--- a/common/gdi_canvas.c
+++ b/common/gdi_canvas.c
@ -308,19 +308,16 @@ uint32_t raster_ops[] = {
    0x00FF0062 // 1 WHITENESS
 };

-static void set_path(GdiCanvas *canvas, void *addr)
+static void set_path(GdiCanvas *canvas, SpicePath *s)
 {
-    uint32_t* data_size = (uint32_t*)addr;
-    uint32_t more = *data_size;
+    SpicePathSeg* seg = s->segments;
+    int i;

-    SpicePathSeg* seg = (SpicePathSeg*)(data_size + 1);
-
-    do {
+    for (i = 0; i < s->num_segments; i++) {
        uint32_t flags = seg->flags;
        SpicePointFix* point = (SpicePointFix*)seg->data;
        SpicePointFix* end_point = point + seg->count;
        ASSERT(point < end_point);
-        more -= ((unsigned long)end_point - (unsigned long)seg);
        seg = (SpicePathSeg*)end_point;

        if (flags & SPICE_PATH_BEGIN) {
@ -371,7 +368,7 @@ static void set_path(GdiCanvas *canvas, void *addr)
            }
        }

-    } while (more);
+    }
 }

 static void set_scale_mode(GdiCanvas *canvas, uint8_t scale_mode)
@ -1799,7 +1796,7 @@ static void gdi_canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox, S
    }
    prev_hpen = (HPEN)SelectObject(canvas->dc, hpen);

-    set_path(canvas, SPICE_GET_ADDRESS(stroke->path));
+    set_path(canvas, stroke->path);

    StrokePath(canvas->dc);

--- a/common/gl_canvas.c
+++ b/common/gl_canvas.c
@ -114,15 +114,13 @@ static pixman_image_t *canvas_surf_to_trans_surf(GLCImage *image,
 static GLCPath get_path(GLCanvas *canvas, SpicePath *s)
 {
    GLCPath path = glc_path_create(canvas->glc);
-    uint32_t more = s->size;
+    int i;
    SpicePathSeg* seg = s->segments;

-    do {
+    for (i = 0; i < s->num_segments; i++) {
        uint32_t flags = seg->flags;
        SpicePointFix* point = seg->points;
        SpicePointFix* end_point = point + seg->count;
-        ASSERT(point < end_point);
-        more -= ((unsigned long)end_point - (unsigned long)seg);
        seg = (SpicePathSeg*)end_point;

        if (flags & SPICE_PATH_BEGIN) {
@ -148,7 +146,7 @@ static GLCPath get_path(GLCanvas *canvas, SpicePath *s)
                glc_path_close(path);
            }
        }
-    } while (more);
+    }

    return path;
 }
@ -621,7 +619,7 @@ static void gl_canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox, Sp
    }
    glc_set_line_width(canvas->glc, fix_to_double(stroke->attr.width));

-    path = get_path(canvas, SPICE_GET_ADDRESS(stroke->path));
+    path = get_path(canvas, stroke->path);
    glc_stroke_path(canvas->glc, path);
    glc_path_destroy(path);
 }
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@ -604,7 +604,7 @@ def read_array_len(writer, prefix, array, dest, scope, handles_bytes = False):
    elif array.is_bytes_length():
        if not handles_bytes:
            raise NotImplementedError("handling of bytes() not supported here yet")
-        writer.assign(nelements, dest.get_ref(array.size[1]))
+        writer.assign(nelements, array.size[1])
    else:
        raise NotImplementedError("TODO array size type not handled yet")
    return nelements
@ -714,10 +714,15 @@ def write_array_parser(writer, nelements, array, dest, scope):
        writer.increment("end", nelements)
    else:
        if is_byte_size:
+            real_nelements = nelements[:-len("nbytes")] + "nelements"
            scope.variable_def("uint8_t *", "array_end")
+            scope.variable_def("uint32_t", real_nelements)
            writer.assign("array_end", "end + %s" % nelements)
+            writer.assign(real_nelements, 0)
        with writer.index(no_block = is_byte_size) as index:
            with writer.while_loop("end < array_end") if is_byte_size else writer.for_loop(index, nelements) as array_scope:
+                if is_byte_size:
+                    writer.increment(real_nelements, 1)
                if element_type.is_primitive():
                    writer.statement("*(%s *)end = consume_%s(&in)" % (element_type.c_type(), element_type.primitive_type()))
                    writer.increment("end", element_type.sizeof())
@ -725,6 +730,8 @@ def write_array_parser(writer, nelements, array, dest, scope):
                    dest2 = dest.child_at_end(writer, element_type)
                    dest2.reuse_scope = array_scope
                    write_container_parser(writer, element_type, dest2)
+        if is_byte_size:
+            writer.assign(dest.get_ref(array.size[2]), real_nelements)

 def write_parse_pointer(writer, t, at_end, as_c_ptr, dest, member_name, scope):
        target_type = t.target_type
@ -766,7 +773,12 @@ def write_member_parser(writer, container, member, dest, scope):
            writer.statement("*(%s *)end = consume_%s(&in)" % (t.c_type(), t.primitive_type()))
            writer.increment("end", t.sizeof())
        else:
-            writer.assign(dest.get_ref(member.name), "consume_%s(&in)" % (t.primitive_type()))
+            if member.has_attr("bytes_count"):
+                scope.variable_def("uint32_t", member.name);
+                dest_var = member.name
+            else:
+                dest_var = dest.get_ref(member.name)
+            writer.assign(dest_var, "consume_%s(&in)" % (t.primitive_type()))
        #TODO validate e.g. flags and enums
    elif t.is_array():
        nelements = read_array_len(writer, member.name, t, dest, scope, handles_bytes = True)
--- a/python_modules/marshal.py
+++ b/python_modules/marshal.py
@ -165,7 +165,7 @@ def get_array_size(array, container_src):
        else:
            return "(((%s * %s + 7) / 8 ) * %s)" % (bpp, width_v, rows_v)
    elif array.is_bytes_length():
-        return container_src.get_ref(array.size[1])
+        return container_src.get_ref(array.size[2])
    else:
        raise NotImplementedError("TODO array size type not handled yet")

@ -179,20 +179,18 @@ def write_array_marshaller(writer, at_end, member, array, container_src, scope):
    nelements = get_array_size(array, container_src)
    is_byte_size = array.is_bytes_length()

-    if is_byte_size:
-        element = "%s__bytes" % member.name
-    else:
-        element = "%s__element" % member.name
+    element = "%s__element" % member.name

    if not at_end:
        writer.assign(element, container_src.get_ref(member.name))

    if is_byte_size:
-        scope.variable_def("size_t", "array_end")
-        writer.assign("array_end", "spice_marshaller_get_size(m) + %s" % nelements)
+        size_start_var = "%s__size_start" % member.name
+        scope.variable_def("size_t", size_start_var)
+        writer.assign(size_start_var, "spice_marshaller_get_size(m)")

-    with writer.index(no_block = is_byte_size) as index:
-        with writer.while_loop("spice_marshaller_get_size(m) < array_end") if is_byte_size else writer.for_loop(index, nelements) as array_scope:
+    with writer.index() as index:
+        with writer.for_loop(index, nelements) as array_scope:
            array_scope.variable_def(element_type.c_type() + " *", element)
            if at_end:
                writer.assign(element, "(%s *)end" % element_type.c_type())
@ -210,6 +208,12 @@ def write_array_marshaller(writer, at_end, member, array, container_src, scope):
            if not at_end:
                writer.statement("%s++" % element)

+    if is_byte_size:
+        size_var = member.container.lookup_member(array.size[1])
+        size_var_type = size_var.member_type
+        var = "%s__ref" % array.size[1]
+        writer.statement("spice_marshaller_set_%s(m, %s, spice_marshaller_get_size(m) - %s)" % (size_var_type.primitive_type(), var, size_start_var))
+
 def write_switch_marshaller(writer, container, switch, src, scope):
    var = container.lookup_member(switch.variable)
    var_type = var.member_type
@ -289,6 +293,11 @@ def write_member_marshaller(writer, container, member, src, scope):
    elif t.is_primitive():
        if member.has_attr("zero"):
            writer.statement("spice_marshaller_add_%s(m, 0)" % (t.primitive_type()))
+        if member.has_attr("bytes_count"):
+            var = "%s__ref" % member.name
+            scope.variable_def("void *", var)
+            writer.statement("%s = spice_marshaller_add_%s(m, %s)" % (var, t.primitive_type(), 0))
+
        elif member.has_end_attr():
            writer.statement("spice_marshaller_add_%s(m, *(%s_t *)end)" % (t.primitive_type(), t.primitive_type()))
            writer.increment("end", t.sizeof())
--- a/python_modules/spice_parser.py
+++ b/python_modules/spice_parser.py
@ -87,7 +87,7 @@ def SPICE_BNF():
        attribute = Group(Combine ("@" + identifier) + Optional(lparen + delimitedList(attributeValue) + rparen))
        attributes = Group(ZeroOrMore(attribute))
        arraySizeSpecImage = Group(image_size_ + lparen + integer + comma + identifier + comma + identifier + rparen)
-        arraySizeSpecBytes = Group(bytes_ + lparen + identifier + rparen)
+        arraySizeSpecBytes = Group(bytes_ + lparen + identifier + comma + identifier + rparen)
        arraySizeSpecCString = Group(cstring_ + lparen + rparen)
        arraySizeSpec = lbrack + Optional(identifier ^ integer ^ arraySizeSpecImage ^ arraySizeSpecBytes ^arraySizeSpecCString, default="") + rbrack
        variableDef = Group(typeSpec + Optional("*", default=None) + identifier + Optional(arraySizeSpec, default=None) + attributes - semi) \
--- a/server/Makefile.am
+++ b/server/Makefile.am
@ -22,7 +22,7 @@ spice_built_sources = generated_marshallers.c generated_marshallers.h generated_
 generated_demarshallers.c: $(top_srcdir)/spice.proto
 	$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-demarshallers --server --include red_common.h $(top_srcdir)/spice.proto generated_demarshallers.c

-STRUCTS=-M String -M Rect -M Point -M DisplayBase -M Fill -M Opaque -M Copy -M Blend -M Blackness -M Whiteness -M Invers -M Rop3 -M Stroke -M Text -M Transparent -M AlphaBlnd -M PathSegment
+STRUCTS=-M String -M Rect -M Point -M DisplayBase -M Fill -M Opaque -M Copy -M Blend -M Blackness -M Whiteness -M Invers -M Rop3 -M Stroke -M Text -M Transparent -M AlphaBlnd
 generated_marshallers.c: $(top_srcdir)/spice.proto
 	$(PYTHON) $(top_srcdir)/spice_codegen.py --include red_common.h --generate-marshallers $(STRUCTS) --server $(top_srcdir)/spice.proto generated_marshallers.c

--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@ -153,8 +153,10 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
    bool free_data;
    QXLPath *qxl;
    SpicePath *red;
-    size_t size;
+    size_t size, mem_size, mem_size2, dsize;
+    int n_segments;
    int i;
+    uint32_t count;

    qxl = (QXLPath *)get_virt(slots, addr, sizeof(*qxl), group_id);
    size = red_get_data_chunks_ptr(slots, group_id,
@ -163,17 +165,44 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
    data = red_linearize_chunk(&chunks, size, &free_data);
    red_put_data_chunks(&chunks);

-    ASSERT(qxl->data_size == size);
-    ASSERT(sizeof(QXLPathSeg) == sizeof(SpicePathSeg)); /* FIXME */
-    red = spice_malloc(sizeof(*red) + size);
-    red->size = qxl->data_size;
+    n_segments = 0;
+    mem_size = sizeof(*red);
+
+    start = (QXLPathSeg*)data;
+    end = (QXLPathSeg*)(data + size);
+    while (start < end) {
+        n_segments++;
+        count = start->count;
+        mem_size += sizeof(SpicePathSeg) + count * sizeof(SpicePointFix);
+        start = (QXLPathSeg*)(&start->points[count]);
+    }
+
+    red = spice_malloc(mem_size);
+    red->num_segments = n_segments;

    start = (QXLPathSeg*)data;
    end = (QXLPathSeg*)(data + size);
    seg = red->segments;
+    n_segments = 0;
+    mem_size2 = sizeof(*red);
    while (start < end) {
+        n_segments++;
+        count = start->count;
+
+        /* Protect against overflow in size calculations before
+           writing to memory */
+        ASSERT(mem_size2 + sizeof(SpicePathSeg) > mem_size2);
+        mem_size2  += sizeof(SpicePathSeg);
+        ASSERT(count < UINT32_MAX / sizeof(SpicePointFix));
+        dsize = count * sizeof(SpicePointFix);
+        ASSERT(mem_size2 + dsize > mem_size2);
+        mem_size2  += dsize;
+
+        /* Verify that we didn't overflow due to guest changing data */
+        ASSERT(mem_size2 <= mem_size);
+
        seg->flags = start->flags;
-        seg->count = start->count;
+        seg->count = count;
        for (i = 0; i < seg->count; i++) {
            seg->points[i].x = start->points[i].x;
            seg->points[i].y = start->points[i].y;
@ -181,6 +210,8 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
        start = (QXLPathSeg*)(&start->points[i]);
        seg = (SpicePathSeg*)(&seg->points[i]);
    }
+    /* Ensure guest didn't tamper with segment count */
+    ASSERT(n_segments == red->num_segments);

    if (free_data) {
        free(data);
--- a/server/red_worker.c
+++ b/server/red_worker.c
@ -2291,10 +2291,29 @@ static inline void __current_add_drawable(RedWorker *worker, Drawable *drawable,

 static int is_equal_path(RedWorker *worker, SpicePath *path1, SpicePath *path2)
 {
-    if (path1->size != path2->size)
-        return FALSE;
-    if (memcmp(path1->segments, path2->segments, path1->size) != 0)
+    SpicePathSeg *seg1, *seg2;
+    int i, j;
+
+    if (path1->num_segments != path2->num_segments)
        return FALSE;
+
+    seg1 = &path1->segments[0];
+    seg2 = &path2->segments[0];
+    for (i = 0; i < path1->num_segments; i++) {
+        if (seg1->flags != seg2->flags ||
+            seg1->count != seg2->count) {
+            return FALSE;
+        }
+        for (j = 0; j < seg1->count; j++) {
+            if (seg1->points[j].x != seg2->points[j].x ||
+                seg1->points[j].y != seg2->points[j].y) {
+                return FALSE;
+            }
+        }
+        seg1 = (SpicePathSeg*)(&seg1->points[seg1->count]);
+        seg2 = (SpicePathSeg*)(&seg2->points[seg2->count]);
+    }
+
    return TRUE;
 }

@ -5121,18 +5140,6 @@ static void add_buf_from_info(RedChannel *channel, SpiceMarshaller *m, AddBufInf
 }


-static void fill_path(SpiceMarshaller *m, SpicePath *path)
-{
-    SpicePathSeg *start, *end;
-
-    spice_marshaller_add_uint32(m, path->size);
-    start = path->segments;
-    end = (SpicePathSeg*)((uint8_t*)(path->segments) + path->size);
-    while (start < end) {
-        start = spice_marshall_PathSegment(m, start);
-    }
-}
-
 static void fill_str(DisplayChannel *display_channel, SpiceMarshaller *m,
                     QXLPHYSICAL in_str, uint32_t group_id)
 {
@ -8004,7 +8011,7 @@ static void red_send_qxl_draw_stroke(RedWorker *worker,
                          &style_out,
                          &brush_pat_out);

-    fill_path(path_out, stroke.path);
+    spice_marshall_Path(path_out, stroke.path);
    fill_attr(display_channel, style_out, &stroke.attr, item->group_id);
    if (brush_pat_out) {
        fill_bits(display_channel, brush_pat_out, stroke.brush.u.pattern.pat, item, FALSE);
--- a/spice.proto
+++ b/spice.proto
@ -404,8 +404,8 @@ struct PathSegment {
 }  @ctype(SpicePathSeg);

 struct Path {
-    uint32 size;
-    PathSegment segments[bytes(size)] @end;
+    uint32 num_segments;
+    PathSegment segments[num_segments] @end;
 };

 struct Clip {
--- a/spice1.proto
+++ b/spice1.proto
@ -374,8 +374,8 @@ struct PathSegment {
 }  @ctype(SpicePathSeg);

 struct Path {
-    uint32 size;
-    PathSegment segments[bytes(size)] @end;
+    uint32 segments_size @bytes_count;
+    PathSegment segments[bytes(segments_size, num_segments)] @end;
 };

 struct Clip {