>
> This is currently unlikely to happen since we communicate over a pipe
> and the pipe buffer is sufficiently large to avoid splitting the
> message. But for completeness, we should handle this scenario.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> Since v1:
> - updates from Frediano's review
> - bumps spice-protocol requirement for
> STREAM_MSG_CAPABILITIES_MAX_BYTES symbol
> - change msg_pos from uint8_t to uint32_t
> - move reset of msg_pos to calling function
> (stream_device_port_event())
>
> configure.ac | 2 +-
> server/stream-device.c | 31 ++++++++++++++++++++++---------
> 2 files changed, 23 insertions(+), 10 deletions(-)
>
I would propose a challenge:
- found the bug;
- write an automatic test to prove the bug;
- fix the bug.
(not in strictly order).
Nothing personal here, just that would be great to have more
automated testing and thinking with test in mind helps.
I'll try to write a test (I've already made the other 2 points).
> diff --git a/configure.ac b/configure.ac
> index fb266ad4c..3401dba83 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -156,7 +156,7 @@ AS_IF([test x"$have_smartcard" = "xyes"], [
> AS_VAR_APPEND([SPICE_REQUIRES], [" libcacard >= 0.1.2"])
> ])
>
> -SPICE_PROTOCOL_MIN_VER=0.12.13
> +SPICE_PROTOCOL_MIN_VER=0.12.14
> PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >=
> $SPICE_PROTOCOL_MIN_VER])
> AC_SUBST([SPICE_PROTOCOL_MIN_VER])
>
> diff --git a/server/stream-device.c b/server/stream-device.c
> index fc5b50659..efa6d8db5 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -42,6 +42,12 @@ struct StreamDevice {
>
> StreamDevHeader hdr;
> uint8_t hdr_pos;
> + union {
> + StreamMsgFormat format;
> + StreamMsgCapabilities capabilities;
> + uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> + } msg;
> + uint32_t msg_pos;
> bool has_error;
> bool opened;
> bool flow_stopped;
> @@ -155,19 +161,25 @@ handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin, const char *
> static bool
> handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> - StreamMsgFormat fmt;
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> - int n = sif->read(sin, (uint8_t *) &fmt, sizeof(fmt));
> - if (n == 0) {
> - return false;
> - }
> - if (n != sizeof(fmt)) {
> +
> + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> + spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> +
> + int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> sizeof(StreamMsgFormat) - dev->msg_pos);
> + if (n < 0) {
> return handle_msg_invalid(dev, sin, NULL);
> }
> - fmt.width = GUINT32_FROM_LE(fmt.width);
> - fmt.height = GUINT32_FROM_LE(fmt.height);
> - stream_channel_change_format(dev->stream_channel, &fmt);
>
> + dev->msg_pos += n;
> +
> + if (dev->msg_pos < sizeof(StreamMsgFormat)) {
> + return false;
> + }
> +
> + dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width);
> + dev->msg.format.height = GUINT32_FROM_LE(dev->msg.format.height);
> + stream_channel_change_format(dev->stream_channel, &dev->msg.format);
> return true;
> }
>
> @@ -334,6 +346,7 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t
> event)
> allocate_channels(dev);
> }
> dev->hdr_pos = 0;
> + dev->msg_pos = 0;
> dev->has_error = false;
> dev->flow_stopped = false;
> red_char_device_reset(char_dev);
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel