[Spice-devel] [PATCH 3.1/12] Store QXLInstance in CursorItem
Frediano Ziglio
fziglio at redhat.com
Mon Nov 2 02:58:28 PST 2015
>
> On Fri, 2015-10-30 at 03:24 -0400, Frediano Ziglio wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >
> > > Doing so allows us to remove the extra QXLInstance parameter from
> > > cursor_item_unref() and makes the code a bit cleaner.
> > >
> > > Also add cursor_item_ref().
> > >
> > > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > ---
> > > server/cursor-channel.c | 70
> > > +++++++++++++++++++++++++++----------------------
> > > server/cursor-channel.h | 9 ++++---
> > > 2 files changed, 44 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index 6cc2b93..2d7afc6 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -25,55 +25,58 @@
> > > #include "cache_item.tmpl.c"
> > > #undef CLIENT_CURSOR_CACHE
> > >
> > > -static inline CursorItem *alloc_cursor_item(void)
> > > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd,
> > > uint32_t
> > > group_id)
> > > {
> > > CursorItem *cursor_item;
> > >
> > > + spice_return_val_if_fail(cmd != NULL, NULL);
> > > +
> > > cursor_item = g_slice_new0(CursorItem);
> > > + cursor_item->qxl = qxl;
> > > cursor_item->refs = 1;
> > > + cursor_item->group_id = group_id;
> > > + cursor_item->red_cursor = cmd;
> > >
> > > return cursor_item;
> > > }
> > >
> > > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> > > +CursorItem *cursor_item_ref(CursorItem *item)
> > > {
> > > - CursorItem *cursor_item;
> > > -
> > > - spice_return_val_if_fail(cmd != NULL, NULL);
> > > - cursor_item = alloc_cursor_item();
> > > + spice_return_val_if_fail(item != NULL, NULL);
> > > + spice_return_val_if_fail(item->refs > 0, NULL);
> >
> > This catch a memory corruption, should abort the program,
> > I suggest spice_error, spice_critical or spice_assert.
>
> It can suggest memory corruption, yes. But I'm not sure that it should
> be fatal. This is exactly the same behavior as g_object_ref(), so I
> don't see any good reason to make this warning fatal when the exact
> same problem with a gobject will not be fatal.
>
Although glib is now "server aware" you have to consider was born as a
client library where some problem are considered in a different way.
Also GObject pointers are a bit more complicated (for instance they support
weak pointers) that could lead to some temporary strange state.
The counters in our code should be handled in a single thread (from worker),
start from 1, incremented for ref, decremented and object freed on zero.
If everything follows these rules counters are always >0.
If they became <= 0 it means to me:
- some unrelated memory corruption (counters written from unexpected code,
physical memory errors, invalid dma transfers or OS bugs). Not the type
of errors you can handle in a safe way;
- counter was incremented till overflow. You are spending lot of memory
with a lot of objects (there are at least 2^31 pointers to this object!)
something is really wrong in your program; DoS is likely to happen;
- counter was decremented more then expected. You have pointer to now freed
data. Dandling pointers are bound to heap memory corruptions which is
a security problem.
In a server context I don't see the point to keep the program going on these
conditions beside opening the doors to hackers.
> >
> > >
> > > - cursor_item->group_id = group_id;
> > > - cursor_item->red_cursor = cmd;
> > > + item->refs++;
> > >
> > > - return cursor_item;
> > > + return item;
> > > }
> > >
> > > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > > +void cursor_item_unref(CursorItem *item)
> > > {
> > > - if (!--cursor->refs) {
> > > - QXLReleaseInfoExt release_info_ext;
> > > - RedCursorCmd *cursor_cmd;
> > > -
> > > - cursor_cmd = cursor->red_cursor;
> > > - release_info_ext.group_id = cursor->group_id;
> > > - release_info_ext.info = cursor_cmd->release_info;
> > > - qxl->st->qif->release_resource(qxl, release_info_ext);
> > > - red_put_cursor_cmd(cursor_cmd);
> > > - free(cursor_cmd);
> > > -
> > > - g_slice_free(CursorItem, cursor);
> > > - }
> > > + QXLReleaseInfoExt release_info_ext;
> > > + RedCursorCmd *cursor_cmd;
> > > +
> > > + spice_return_if_fail(item != NULL);
> > > +
> > > + if (--item->refs)
> > > + return;
> > > +
> > > + cursor_cmd = item->red_cursor;
> > > + release_info_ext.group_id = item->group_id;
> > > + release_info_ext.info = cursor_cmd->release_info;
> > > + item->qxl->st->qif->release_resource(item->qxl,
> > > release_info_ext);
> > > + red_put_cursor_cmd(cursor_cmd);
> > > + free(cursor_cmd);
> > > +
> > > + g_slice_free(CursorItem, item);
> > > +
> > > }
> > >
> > > static void cursor_set_item(CursorChannel *cursor, CursorItem
> > > *item)
> > > {
> > > if (cursor->item)
> > > - cursor_item_unref(red_worker_get_qxl(cursor
> > > ->common.worker),
> > > cursor->item);
> > > -
> > > - if (item)
> > > - item->refs++;
> > > + cursor_item_unref(cursor->item);
> > >
> > > - cursor->item = item;
> > > + cursor->item = item ? cursor_item_ref(item) : NULL;
> > > }
> > >
> >
> > Are these tests worth doing or can we assume that a NULL pointer is
> > expected (like
> > unref(NULL) and ref(NULL) do nothing) ? This would make
> > cursor_set_item like
> >
> > static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
> > {
> > cursor_item_unref(cursor->item);
> >
> > cursor->item = cursor_item_ref(item);
> > }
>
>
> I'd prefer to stay closer to the semantics of g_object_ref/unref(),
> which warn if the argument is not a valid GObject.
>
> >
> >
> > On naming, sometimes is cursor_item sometimes item. I would prefer
> > cursor_item.
> >
> > > static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void
> > > *data, int
> > > num)
> > > @@ -155,7 +158,7 @@ static void
> > > put_cursor_pipe_item(CursorChannelClient
> > > *ccc, CursorPipeItem *pipe_
> > >
> > > spice_assert(!pipe_item_is_linked(&pipe_item->base));
> > >
> > > - cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> > > pipe_item->cursor_item);
> > > + cursor_item_unref(pipe_item->cursor_item);
> > > free(pipe_item);
> > > }
> > >
> > > @@ -411,7 +414,11 @@ void cursor_channel_process_cmd(CursorChannel
> > > *cursor,
> > > RedCursorCmd *cursor_cmd,
> > > CursorItem *cursor_item;
> > > int cursor_show = FALSE;
> > >
> > > - cursor_item = cursor_item_new(cursor_cmd, group_id);
> > > + spice_return_if_fail(cursor);
> > > + spice_return_if_fail(cursor_cmd);
> > > +
> > > + cursor_item = cursor_item_new(red_worker_get_qxl(cursor
> > > ->common.worker),
> > > + cursor_cmd, group_id);
> > >
> > > switch (cursor_cmd->type) {
> > > case QXL_CURSOR_SET:
> > > @@ -439,7 +446,8 @@ void cursor_channel_process_cmd(CursorChannel
> > > *cursor,
> > > RedCursorCmd *cursor_cmd,
> > > red_channel_pipes_new_add(&cursor->common.base,
> > > new_cursor_pipe_item,
> > > (void*)cursor_item);
> > > }
> > > - cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> > > cursor_item);
> > > +
> > > + cursor_item_unref(cursor_item);
> > > }
> > >
> > > void cursor_channel_reset(CursorChannel *cursor)
> > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > index 2c19859..8b49df7 100644
> > > --- a/server/cursor-channel.h
> > > +++ b/server/cursor-channel.h
> > > @@ -33,6 +33,7 @@
> > > #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> > >
> > > typedef struct CursorItem {
> > > + QXLInstance *qxl;
> > > uint32_t group_id;
> > > int refs;
> > > RedCursorCmd *red_cursor;
> > > @@ -83,14 +84,14 @@ void cursor_channel_reset
> > > (CursorChannel *cursor);
> > > void cursor_channel_process_cmd (CursorChannel
> > > *cursor,
> > > RedCursorCmd *cursor_cmd,
> > > uint32_t
> > > group_id);
> > >
> > > -CursorItem* cursor_item_new (RedCursorCmd
> > > *cmd, uint32_t
> > > group_id);
> > > -void cursor_item_unref (QXLInstance *qxl,
> > > CursorItem *cursor);
> > > -
> > > -
> > > CursorChannelClient *cursor_channel_client_new(CommonChannel
> > > *common,
> > > RedClient *client,
> > > RedsStream
> > > *stream,
> > > int mig_target,
> > > uint32_t
> > > *common_caps, int
> > > num_common_caps,
> > > uint32_t *caps, int
> > > num_caps);
> > >
> > > +CursorItem* cursor_item_new (QXLInstance *qxl,
> > > RedCursorCmd *cmd, uint32_t group_id);
> > > +CursorItem* cursor_item_ref (CursorItem
> > > *cursor);
> > > +void cursor_item_unref (CursorItem
> > > *cursor);
> > > +
> > > #endif /* CURSOR_CHANNEL_H_ */
> >
Frediano
More information about the Spice-devel
mailing list