[Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities
Frediano Ziglio
fziglio at redhat.com
Wed Mar 21 17:07:25 UTC 2018
>
> > On 20 Mar 2018, at 12:28, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> >>
> >> Looks good, with minor nits.
> >>
> >>> On 19 Mar 2018, at 17:46, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>>
> >>> Handle capabilities from guest device.
> >>> Send capability to the guest when device is opened.
> >>> Currently there's no capabilities set on the message sent.
> >>> On the tests we need to discard the capability message before
> >>> reading the error.
> >>>
> >>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >>> ---
> >>> server/red-stream-device.c | 66
> >>> +++++++++++++++++++++++++++++++++++++--
> >>> server/tests/test-stream-device.c | 22 +++++++++++++
> >>> 2 files changed, 85 insertions(+), 3 deletions(-)
> >>>
> >>> Changes since v1:
> >>> - rebased on master (with minor fix due to rename).
> >>>
> >>> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> >>> index e91df88d..1732b888 100644
> >>> --- a/server/red-stream-device.c
> >>> +++ b/server/red-stream-device.c
> >>> @@ -1,6 +1,6 @@
> >>> /* spice-server character device to handle a video stream
> >>>
> >>> - Copyright (C) 2017 Red Hat, Inc.
> >>> + Copyright (C) 2017-2018 Red Hat, Inc.
> >>>
> >>> This library is free software; you can redistribute it and/or
> >>> modify it under the terms of the GNU Lesser General Public
> >>> @@ -25,6 +25,8 @@
> >>> #include "cursor-channel.h"
> >>> #include "reds.h"
> >>>
> >>> +#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> >>> +
> >>> struct StreamDevice {
> >>> RedCharDevice parent;
> >>>
> >>> @@ -42,6 +44,7 @@ struct StreamDevice {
> >>> bool has_error;
> >>> bool opened;
> >>> bool flow_stopped;
> >>> + uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
> >>> StreamChannel *stream_channel;
> >>> CursorChannel *cursor_channel;
> >>> SpiceTimer *close_timer;
> >>> @@ -61,7 +64,7 @@ typedef bool StreamMsgHandler(StreamDevice *dev,
> >>> SpiceCharDeviceInstance *sin)
> >>> SPICE_GNUC_WARN_UNUSED_RESULT;
> >>>
> >>> static StreamMsgHandler handle_msg_format, handle_msg_data,
> >>> handle_msg_cursor_set,
> >>> - handle_msg_cursor_move;
> >>> + handle_msg_cursor_move, handle_msg_capabilities;
> >>>
> >>> static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> >>> *sin,
> >>> const char *error_msg)
> >>> SPICE_GNUC_WARN_UNUSED_RESULT;
> >>> @@ -147,7 +150,8 @@ stream_device_partial_read(StreamDevice *dev,
> >>> SpiceCharDeviceInstance *sin)
> >>> }
> >>> break;
> >>> case STREAM_TYPE_CAPABILITIES:
> >>> - /* FIXME */
> >>> + handled = handle_msg_capabilities(dev, sin);
> >>> + break;
> >>> default:
> >>> handled = handle_msg_invalid(dev, sin, "Invalid message type");
> >>> break;
> >>> @@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev,
> >>> SpiceCharDeviceInstance *sin)
> >>> return true;
> >>> }
> >>>
> >>> +static bool
> >>> +handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> >>> +{
> >>> + SpiceCharDeviceInterface *sif =
> >>> spice_char_device_get_interface(sin);
> >>> +
> >>> + if (spice_extra_checks) {
>
> As an aside, the fact that you used a lower-case for an enum in violation of
> the existing practice in the code makes this look like a regular test, not a
> configuration test.
>
You are right, maybe upper case would be better.
> >>
> >> Premature optimization.
> >>
> >
> > Extra is not expensive
>
> If it’s not expensive, then what is it? (See my comments in other patch, the
> alternatives to “expensive” in english are “superfluous” or “superior”).
>
> > and code is coherent with other part.
>
> Yes, I’m well aware you consistently ignored my input on this topic earlier
> :-)
>
> To be clear, I”m nacking the “if”, not the assert, in
>
> if (spice_extra_check)
> {
> spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> }
>
> The “if” means “these tests have something special that makes them worth
> skipping by default”. Therefore, the “if” test is basically lying to me :-)
> which is why I want it gone.
>
In this case the reason is "paranoia". The size test is a duplicate,
the type is like a malloc function that is checking if the caller
really wants to allocate memory.
> A wise man once wrote on this list that he would rather have a core than a
> remote execution. I believe that you should listen to him.
>
These tests do not prevent a remote execution. These kind of test are
more used during testing if you really screw up things.
>
>
> >>> + }
> >
> >>> + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >>> + spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> >>> + }
> >>> +
> >>> + if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
> >>> + return handle_msg_invalid(dev, sin, "Wrong size for
> >>> StreamMsgCapabilities");
> >>> + }
> >>> +
> >>> + int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> >>> dev->msg_pos);
> >>> + if (n < 0) {
> >>
> >> Reading the various sif->read, the convention on return values is a bit
> >> unclear. Most other places seem to check for <= 0. Only handle_msg_format
> >> uses < 0. Someone could teach me why?
> >>
> >> Is it possible for sif->read to return 0 on error?
> >
> > No, 0 is 0 byte in the current buffer which does not mean end of file,
> > there's no EAGAIN behaviour.
>
> That’s what I guessed. Thanks for confirming.
>
I think a patch in server/spice-char.h adding some comments would be
great. Whoever wrote these interfaces didn't do it leaving the implementer
to guess or having to look at the implementation for these details which
are far from easy to grasp. A read that by standard is not blocking or
behave differently based on the state of the device is far from what you
expect.
> > Basically with <=0 you handle either 0 bytes or error while with <0 only
> > errors.
>
> So here, if n < dev->hdr.size - dev->msg.pos case, we fall through and we
> retry later because we return false. Got it.
>
> >
> >> Is it possible for sif->read to return less than the requested size (e.g.
> >> EINTR)?
> >>
> >
> > There's no EINTR but what can happen is that guest did a partial write
> > or the buffer was full so the write was truncated. The interface is not
> > blocking so partial read are normal.
>
> This logic is repeated many times. A bit WET to my taste. But OK, at least
> now I understand.
>
Yes, I'm realizing I did a bit of too much copy and paste too.
> >
> >>
> >>> + return handle_msg_invalid(dev, sin, NULL);
> >>> + }
> >>> +
> >>> + dev->msg_pos += n;
> >>> +
> >>> + if (dev->msg_pos < dev->hdr.size) {
> >>> + return false;
> >>> + }
> >>> +
> >>> + // copy only capabilities we care about
> >>
> >> I think it’s “capabilities we know about”, since ifsd we get extra, it’s
> >> probably stuff added after this version.
> >>
> >
> > updated
> >
> >>> + memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
> >>> + memcpy(dev->guest_capabilities, dev->msg->buf,
> >>> MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));
> >>> +
> >>> + return true;
> >>> +}
> >>> +
> >>> static bool
> >>> handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> >>> {
> >>> @@ -586,6 +622,28 @@ char_device_set_state(RedCharDevice *char_dev, int
> >>> state)
> >>> }
> >>> }
> >>>
> >>> +static void
> >>> +send_capabilities(RedCharDevice *char_dev)
> >>> +{
> >>> + int msg_size = MAX_GUEST_CAPABILITIES_BYTES;
> >>> + int total_size = sizeof(StreamDevHeader) + msg_size;
> >>> +
> >>> + RedCharDeviceWriteBuffer *buf =
> >>> + red_char_device_write_buffer_get_server_no_token(char_dev,
> >>> total_size);
> >>> + buf->buf_used = total_size;
> >>> +
> >>> + StreamDevHeader *const hdr = (StreamDevHeader *)buf->buf;
> >>> + hdr->protocol_version = STREAM_DEVICE_PROTOCOL;
> >>> + hdr->padding = 0;
> >>> + hdr->type = GUINT16_TO_LE(STREAM_TYPE_CAPABILITIES);
> >>> + hdr->size = GUINT32_TO_LE(msg_size);
> >>
> >> We don’t have a macro/function taking care of filling a StreamDevHeader?
> >>
> >
> > No, there are not much messages for the guest, can be introduced
>
> Should be introduced according to DRY principle.
>
I'll add one as a preparation patch.
> >
> >>> +
> >>> + StreamMsgCapabilities *const caps = (StreamMsgCapabilities
> >>> *)(hdr+1);
> >>> + memset(caps, 0, msg_size);
> >>> +
> >>> + red_char_device_write_buffer_add(char_dev, buf);
> >>> +}
> >>> +
> >>> static void
> >>> stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
> >>> {
> >>> @@ -599,6 +657,8 @@ stream_device_port_event(RedCharDevice *char_dev,
> >>> uint8_t event)
> >>> dev->opened = (event == SPICE_PORT_EVENT_OPENED);
> >>> if (dev->opened) {
> >>> stream_device_create_channel(dev);
> >>> +
> >>> + send_capabilities(char_dev);
> >>> }
> >>> dev->hdr_pos = 0;
> >>> dev->msg_pos = 0;
> >>> diff --git a/server/tests/test-stream-device.c
> >>> b/server/tests/test-stream-device.c
> >>> index 3c9209a4..43011f9d 100644
> >>> --- a/server/tests/test-stream-device.c
> >>> +++ b/server/tests/test-stream-device.c
> >>> @@ -135,12 +135,33 @@ static uint8_t *add_format(uint8_t *p, uint32_t w,
> >>> uint32_t h, SpiceVideoCodecTy
> >>> return p + sizeof(fmt);
> >>> }
> >>>
> >>> +// remove capabilities from server reply
> >>
> >> The name already says that (except it uses “consume” and not “remove”.
> >> The comment would be more helpful to me if it explained why this is
> >> necessary.
> >> Based on the fact this is to make a test pass, I think that
> >> “skip_server_capabilities” or “ignore_server_capbilities” would be a
> >> better
> >> name.
> >>
> >
> > "consume" is used in the demarshalling code, I think is more appropriate
> > as indicate that data is removed from the buffer.
> >
> > Yes, comment does not add much, I'll remove
>
> Or you could replace it with “capabilities are not tested at the moment, we
> ignore them” or something like that.
>
Looks like more a comment for the caller or a use case.
I'll try to add.
> >
> >>> +static void
> >>> +consume_server_capabilities(void)
> >>> +{
> >>> + StreamDevHeader hdr;
> >>> +
> >>> + if (vmc_write_pos == 0) {
> >>> + return;
> >>> + }
> >>> + g_assert(vmc_write_pos >= sizeof(hdr));
> >>> +
> >>> + memcpy(&hdr, vmc_write_buf, sizeof(hdr));
> >>> + if (GUINT16_FROM_LE(hdr.type) == STREAM_TYPE_CAPABILITIES) {
> >>> + g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos -
> >>> sizeof(hdr));
> >>> + vmc_write_pos -= GUINT32_FROM_LE(hdr.size) + sizeof(hdr);
> >>> + memmove(vmc_write_buf, vmc_write_buf + GUINT32_FROM_LE(hdr.size)
> >>> +
> >>> sizeof(hdr), vmc_write_pos);
> >>> + }
> >>> +}
> >>> +
> >>> // check we have an error message on the write buffer
> >>> static void
> >>> check_vmc_error_message(void)
> >>> {
> >>> StreamDevHeader hdr;
> >>>
> >>> + consume_server_capabilities();
> >>> +
> >>> g_assert(vmc_write_pos >= sizeof(hdr));
> >>>
> >>> memcpy(&hdr, vmc_write_buf, sizeof(hdr));
> >>> @@ -245,6 +266,7 @@ static void test_stream_device_unfinished(void)
> >>> g_assert(message_sizes_curr - message_sizes == 1);
> >>>
> >>> // we should have no data from the device
> >>> + consume_server_capabilities();
> >>> g_assert_cmpint(vmc_write_pos, ==, 0);
> >>>
> >>> test_destroy(test);
> >
Frediano
More information about the Spice-devel
mailing list