>
> On Tue, 2015-11-17 at 16:37 +0000, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > Make sure an allocated buffer is correctly referenced by the marshaller,
> > and can't be free and reused by mistake. Simplify the code by using
> > GSlice
> > ---
> > server/display-channel.h | 3 --
> > server/red_worker.c | 111
> > +++++++++++-----------------------------------
> > -
> > 2 files changed, 25 insertions(+), 89 deletions(-)
> >
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index a1e5947..b0af138 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -81,7 +81,6 @@ struct RedCompressBuf {
> > uint8_t bytes[RED_COMPRESS_BUF_SIZE];
> > uint32_t words[RED_COMPRESS_BUF_SIZE / 4];
> > } buf;
> > - RedCompressBuf *next;
> > RedCompressBuf *send_next;
> > };
> >
> > @@ -192,8 +191,6 @@ struct DisplayChannelClient {
> > uint32_t stream_outbuf_size;
> > uint8_t *stream_outbuf; // caution stream buffer is also used as
> > compress bufs!!!
> >
> > - RedCompressBuf *used_compress_bufs;
> > -
> > FreeList free_list;
> > uint64_t pixmap_cache_items[MAX_DRAWABLE_PIXMAP_CACHE_ITEMS];
> > int num_pixmap_cache_items;
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 5b14836..8e3be51 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -2175,16 +2175,27 @@ static void
> > red_push_surface_image(DisplayChannelClient *dcc, int surface_id)
> > red_channel_client_push(RED_CHANNEL_CLIENT(dcc));
> > }
> >
> > +static RedCompressBuf *compress_buf_new(void)
> > +{
> > + return g_slice_new(RedCompressBuf);
> > +}
> > +
> > +static void compress_buf_free(RedCompressBuf *buf)
> > +{
> > + g_slice_free(RedCompressBuf, buf);
> > +}
> > +
> > static void marshaller_add_compressed(SpiceMarshaller *m,
> > RedCompressBuf *comp_buf, size_t
> > size)
> > {
> > size_t max = size;
> > size_t now;
> > do {
> > - spice_assert(comp_buf);
> > + spice_return_if_fail(comp_buf);
> > now = MIN(sizeof(comp_buf->buf), max);
> > max -= now;
> > - spice_marshaller_add_ref(m, comp_buf->buf.bytes, now);
> > + spice_marshaller_add_ref_full(m, comp_buf->buf.bytes, now,
> > +
> > (spice_marshaller_item_free_func)compress_buf_free, NULL);
>
>
> This part makes me a little nervous. the marshaller will free the data by
> calling compress_buf_free() and passing the address of comp_buf->buf.bytes.
> This
> happens to work because buf.bytes is the very first element of the structure.
> But if anybody changed the RedCompressBuf structure definition, this could
> result in memory corruption. I'd be a little more comfortable if we passed
> 'comp_buf' as the 'opaque' parameter and used that in the free_data function.
> Either that, or add a prominent comment to the RedCompressBuf struct
> definition
> indicating that buf *must* be the first element in the structure so that
> nobody
> accidentally changes that without thinking. The rest of the commit looks
> fine
> to me.
>
I don't even consider the first option.
Also the function cast is not portable (I thing can core on x86!).
I'll write a new version.
Frediano
>
> > comp_buf = comp_buf->send_next;
> > } while (max);
> > }
> > @@ -2219,66 +2230,6 @@ static inline void fill_palette(DisplayChannelClient
> > *dcc,
> > }
> > }
> >
> > -static inline RedCompressBuf
> > *red_display_alloc_compress_buf(DisplayChannelClient *dcc)
> > -{
> > - DisplayChannel *display_channel = DCC_TO_DC(dcc);
> > - RedCompressBuf *ret;
> > -
> > - if (display_channel->free_compress_bufs) {
> > - ret = display_channel->free_compress_bufs;
> > - display_channel->free_compress_bufs = ret->next;
> > - } else {
> > - ret = spice_new(RedCompressBuf, 1);
> > - }
> > -
> > - ret->next = dcc->send_data.used_compress_bufs;
> > - dcc->send_data.used_compress_bufs = ret;
> > - return ret;
> > -}
> > -
> > -static inline void __red_display_free_compress_buf(DisplayChannel *dc,
> > - RedCompressBuf *buf)
> > -{
> > - buf->next = dc->free_compress_bufs;
> > - dc->free_compress_bufs = buf;
> > -}
> > -
> > -static void red_display_free_compress_buf(DisplayChannelClient *dcc,
> > - RedCompressBuf *buf)
> > -{
> > - DisplayChannel *display_channel = DCC_TO_DC(dcc);
> > - RedCompressBuf **curr_used = &dcc->send_data.used_compress_bufs;
> > -
> > - for (;;) {
> > - spice_assert(*curr_used);
> > - if (*curr_used == buf) {
> > - *curr_used = buf->next;
> > - break;
> > - }
> > - curr_used = &(*curr_used)->next;
> > - }
> > - __red_display_free_compress_buf(display_channel, buf);
> > -}
> > -
> > -static void red_display_reset_compress_buf(DisplayChannelClient *dcc)
> > -{
> > - while (dcc->send_data.used_compress_bufs) {
> > - RedCompressBuf *buf = dcc->send_data.used_compress_bufs;
> > - dcc->send_data.used_compress_bufs = buf->next;
> > - __red_display_free_compress_buf(DCC_TO_DC(dcc), buf);
> > - }
> > -}
> > -
> > -static void red_display_destroy_compress_bufs(DisplayChannel
> > *display_channel)
> > -{
> > - spice_assert(!red_channel_is_connected(RED_CHANNEL(display_channel)));
> > - while (display_channel->free_compress_bufs) {
> > - RedCompressBuf *buf = display_channel->free_compress_bufs;
> > - display_channel->free_compress_bufs = buf->next;
> > - free(buf);
> > - }
> > -}
> > -
> > /******************************************************
> > * Global lz red drawables routines
> > *******************************************************/
> > @@ -2606,9 +2557,7 @@ static int encoder_usr_more_space(EncoderData
> > *enc_data,
> > uint8_t **io_ptr)
> > {
> > RedCompressBuf *buf;
> >
> > - if (!(buf = red_display_alloc_compress_buf(enc_data->dcc))) {
> > - return 0;
> > - }
> > + buf = compress_buf_new();
> > enc_data->bufs_tail->send_next = buf;
> > enc_data->bufs_tail = buf;
> > buf->send_next = NULL;
> > @@ -2868,7 +2817,7 @@ static inline int
> > red_glz_compress_image(DisplayChannelClient *dcc,
> > int glz_size;
> > int zlib_size;
> >
> > - glz_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + glz_data->data.bufs_tail = compress_buf_new();
> > glz_data->data.bufs_head = glz_data->data.bufs_tail;
> >
> > if (!glz_data->data.bufs_head) {
> > @@ -2904,7 +2853,7 @@ static inline int
> > red_glz_compress_image(DisplayChannelClient *dcc,
> > #endif
> > zlib_data = &worker->zlib_data;
> >
> > - zlib_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + zlib_data->data.bufs_tail = compress_buf_new();
> > zlib_data->data.bufs_head = zlib_data->data.bufs_tail;
> >
> > if (!zlib_data->data.bufs_head) {
> > @@ -2927,7 +2876,7 @@ static inline int
> > red_glz_compress_image(DisplayChannelClient *dcc,
> > while (zlib_data->data.bufs_head) {
> > RedCompressBuf *buf = zlib_data->data.bufs_head;
> > zlib_data->data.bufs_head = buf->send_next;
> > - red_display_free_compress_buf(dcc, buf);
> > + compress_buf_free(buf);
> > }
> > goto glz;
> > }
> > @@ -2966,7 +2915,7 @@ static inline int
> > red_lz_compress_image(DisplayChannelClient *dcc,
> > stat_time_t start_time = stat_now(DCC_TO_DC(dcc)->lz_stat.clock);
> > #endif
> >
> > - lz_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + lz_data->data.bufs_tail = compress_buf_new();
> > lz_data->data.bufs_head = lz_data->data.bufs_tail;
> >
> > if (!lz_data->data.bufs_head) {
> > @@ -2980,7 +2929,7 @@ static inline int
> > red_lz_compress_image(DisplayChannelClient *dcc,
> > while (lz_data->data.bufs_head) {
> > RedCompressBuf *buf = lz_data->data.bufs_head;
> > lz_data->data.bufs_head = buf->send_next;
> > - red_display_free_compress_buf(dcc, buf);
> > + compress_buf_free(buf);
> > }
> > return FALSE;
> > }
> > @@ -3069,7 +3018,7 @@ static int
> > red_jpeg_compress_image(DisplayChannelClient
> > *dcc, SpiceImage *dest,
> > return FALSE;
> > }
> >
> > - jpeg_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + jpeg_data->data.bufs_tail = compress_buf_new();
> > jpeg_data->data.bufs_head = jpeg_data->data.bufs_tail;
> >
> > if (!jpeg_data->data.bufs_head) {
> > @@ -3084,7 +3033,7 @@ static int
> > red_jpeg_compress_image(DisplayChannelClient
> > *dcc, SpiceImage *dest,
> > while (jpeg_data->data.bufs_head) {
> > RedCompressBuf *buf = jpeg_data->data.bufs_head;
> > jpeg_data->data.bufs_head = buf->send_next;
> > - red_display_free_compress_buf(dcc, buf);
> > + compress_buf_free(buf);
> > }
> > return FALSE;
> > }
> > @@ -3186,7 +3135,7 @@ static int
> > red_lz4_compress_image(DisplayChannelClient
> > *dcc, SpiceImage *dest,
> > stat_time_t start_time = stat_now(worker->clockid);
> > #endif
> >
> > - lz4_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + lz4_data->data.bufs_tail = compress_buf_new();
> > lz4_data->data.bufs_head = lz4_data->data.bufs_tail;
> >
> > if (!lz4_data->data.bufs_head) {
> > @@ -3201,7 +3150,7 @@ static int
> > red_lz4_compress_image(DisplayChannelClient
> > *dcc, SpiceImage *dest,
> > while (lz4_data->data.bufs_head) {
> > RedCompressBuf *buf = lz4_data->data.bufs_head;
> > lz4_data->data.bufs_head = buf->send_next;
> > - red_display_free_compress_buf(dcc, buf);
> > + compress_buf_free(buf);
> > }
> > return FALSE;
> > }
> > @@ -3269,13 +3218,8 @@ static inline int
> > red_quic_compress_image(DisplayChannelClient *dcc, SpiceImage
> > return FALSE;
> > }
> >
> > - quic_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> > + quic_data->data.bufs_tail = compress_buf_new();
> > quic_data->data.bufs_head = quic_data->data.bufs_tail;
> > -
> > - if (!quic_data->data.bufs_head) {
> > - return FALSE;
> > - }
> > -
> > quic_data->data.bufs_head->send_next = NULL;
> > quic_data->data.dcc = dcc;
> >
> > @@ -3283,7 +3227,7 @@ static inline int
> > red_quic_compress_image(DisplayChannelClient *dcc, SpiceImage
> > while (quic_data->data.bufs_head) {
> > RedCompressBuf *buf = quic_data->data.bufs_head;
> > quic_data->data.bufs_head = buf->send_next;
> > - red_display_free_compress_buf(dcc, buf);
> > + compress_buf_free(buf);
> > }
> > return FALSE;
> > }
> > @@ -3763,7 +3707,6 @@ static void fill_attr(SpiceMarshaller *m,
> > SpiceLineAttr
> > *attr, uint32_t group_id
> >
> > static inline void red_display_reset_send_data(DisplayChannelClient *dcc)
> > {
> > - red_display_reset_compress_buf(dcc);
> > dcc->send_data.free_list.res->count = 0;
> > dcc->send_data.num_pixmap_cache_items = 0;
> > memset(dcc->send_data.free_list.sync, 0, sizeof(dcc
> > ->send_data.free_list.sync));
> > @@ -5990,14 +5933,10 @@ static void
> > display_channel_client_on_disconnect(RedChannelClient *rcc)
> > red_release_glz(dcc);
> > red_reset_palette_cache(dcc);
> > free(dcc->send_data.stream_outbuf);
> > - red_display_reset_compress_buf(dcc);
> > free(dcc->send_data.free_list.res);
> > dcc_destroy_stream_agents(dcc);
> >
> > // this was the last channel client
> > - if (!red_channel_is_connected(rcc->channel)) {
> > - red_display_destroy_compress_bufs(display);
> > - }
> > spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d",
> > display->drawable_count, worker->red_drawable_count,
> > worker->glz_drawable_count);
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel