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

[Frediano Ziglio]

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

Some images/video can help :-)
- https://twitter.com/timbray/status/822470746773409794?lang=en
- https://www.youtube.com/watch?v=0GypdsJulKE

Writing tests is an hard job. Said that we have integration tests
run by us and some other by our Q/A team.
But usually integration tests are hard and long to run (for instance
as they require some long setup or because they are manual so not
good for a CI).

Having more unit tests would help. How to write unit tests for C?
Well, not easy either. There are ton of tools for it but in many
case they get complicated. There are usually 2 kind of "facilities":
1) unit tests frameworks/libraries/whatever;
2) mock frameworks/tools/whatever.
1) are quite easy to find and basically they allows to define the
tests in some way, helps run them, get some fancy output. Glib
provides this for instance (and we use in spice-server). Personally
I like the idea behind MinUnit framework
(http://www.jera.com/techinfo/jtns/jtn002.html)! Is really 3 lines
of code. Is not much important the unit tests framework, much more
the presence of tests!
2) helps writing tests. Tests are mainly checking that doing something
specific has some results. Technically this is usually defined having
a set of input and output pairs and testing that a given function
returns such outputs given the respective input. But if you starts
considering and input and output include some internal states and
the amount of possible cases you can have you can easily understand
that the problem is impossible to define. So basically what you try
to do is to limit the cases and check what's happens. For instance
if you are trying to implement a strdup like function you want to
check that a malloc function is called or similar. Here came the
mocking, basically replacing some part for checking and to limit
state bloating. But compared to high level languages (like Python)
catching and altering object behaviour is easy while in C is quite
hard. All solutions I found are invasive and depending on some
external tools (like specific linker options).
All that said I don't have a magic answer. automake offers the
possibility to write tests basically calling some executables
(can be compiled or scripts or a mix). Having inside the same
repository make sure they continue to run and running "make check"
that they continue to work.

> 
> > 
> > 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, 

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

[Christophe de Dinechin]

> On 16 Nov 2017, at 10:46, Frediano Ziglio  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 
>> ---
>> 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 *) , 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, );
>> 
>> +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, >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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> 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 
> ---
> 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 *) , 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, );
>  
> +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, >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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel