[Spice-devel] [PATCH spice-server v2] StreamDevice: Handle incomplete reads of StreamMsgFormat

[Christophe de Dinechin]

> On 16 Nov 2017, at 10:46, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> 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 at redhat.com>
>> ---
>> 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.

+1. This is in-line with the consensus that we need better tests.

Any idea how we could test the streaming agent? The test you put
in place with hexdump hints that you had some idea, but I don’t know
what it is.


> 
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel