[Spice-devel] [PATCH spice-streaming-agent v5 2/2] Implement handling of error messages from the server

[Christophe de Dinechin]

> On 3 Mar 2018, at 10:36, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> On 28 Feb 2018, at 13:53, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>>> 
>>> Log error messages from the server to syslog (not aborting the agent).
>>> Limits the messages to 1023 bytes, error messages from the server are
>>> not expected to be longer. In the unlikely case the message is longer,
>>> log the first 1023 bytes and throw an exception (because we don't read
>>> out the rest of the message, causing desynchronization between the
>>> server and the streaming agent).
>>> 
>>> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
>>> ---
>>> src/spice-streaming-agent.cpp | 26 +++++++++++++++++++++++---
>>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index ee57920..954d1b3 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -137,10 +137,30 @@ static void handle_stream_capabilities(uint32_t len)
>>>    }
>>> }
>>> 
>>> -static void handle_stream_error(uint32_t len)
>>> +static void handle_stream_error(size_t len)
>>> {
>>> -    // TODO read message and use it
>>> -    throw std::runtime_error("got an error message from server");
>>> +    if (len < sizeof(StreamMsgNotifyError)) {
>>> +        throw std::runtime_error("Received NotifyError message size " +
>>> std::to_string(len) +
>>> +                                 " is too small (smaller than " +
>>> +
>>> std::to_string(sizeof(StreamMsgNotifyError))
>>> + ")");
>>> +    }
>>> +
>>> +    struct : StreamMsgNotifyError {
>> 
>> :-O
>> 
>> I had t read that twice to make sure that was actually what was written…
>> Barf.
>> 
>> 
>>> +        uint8_t msg[1024];
>> 
>> Hard-coded magic value. There should be a constant for it in stream-device.h.
>> Please add it.
>> 
> 
> Currently the protocol does not define a limit, this is a guest limit but
> probably a safe and reasonable limit for the protocol can be decided.

Problem with this approach is that the guest cannot read anything past any error message that is too large for it. One could 
- define a protocol limit (the simplest)
- skip the extra bytes
- allocate a modest amount for the common case, and dynamically allocate otherwise
- use dynamic storage in all cases

I don’t care either way, but killing the agent if the server sends an error that is too large opens a denial of service window.

> 
>>> +    } msg;
>> 
>> So I was aggravated very recently regarding padding (having to initialize
>> it…), but this patch gets nary a peep and the series is acked and merged
>> right away?
>> 
> 
> Perhaps you lost the mails saying that the protocol structure don't and won't have internal padding.

Only on x86. It has padding on any ABI with a natural 64-bit alignment.

I don’t have an Itanium handy, but computing the offsetof(msg.msg) and offsetof(msg.StreamNotifyError::msg) on a Raspberry Pi using -mstructure-size-boundary=64 yields:

offsetof msg.msg=8
offsetof msg.StreamNotifyError::msg=4


>> When you inherit, the derived members are aligned, and there is potential
>> padding. However, the code as merged relies on msg.msg being identical to
>> msg.StreamMsgNotifyError::msg. In other words, this code implicitly relies
>> on the lack of any padding. Add for example a ‘char c’ or ‘bool b’ after
>> error_code in StreamMsgNotifyError, and suddenly,
> 
> Adding bool or char to StreamMsgNotifyError or a internal padding is
> a bug writing protocol (as already discussed) and also being an ABI now
> we can't change it so there isn't and there won't be any such field.

I was using that as an illustration of the problem.

> 
>> msg.StreamMsgNotifyError::msg is at offset 5 while msg.msg is at offset 8 on
>> my machine, and all your messages will “mysteriously” be off by three bytes.
>> 
> 
> Yes, if you don't know how to write a protocol structure, ignore the ABI and ignore the notes on the protocol file describing it you have this problem.

Why not just use the “packed” attribute?

> 
>> Please fix it.
>> 
> 
> Fix what ?

The non-portable, hard to read code that was introduced by this patch.

For example, use the original msg field, not the derived object’s msg field. If you do that, gcc warns about an out-of-bounds access because the size is 0, so you may need to go through an intermediate pointer.

> 
>>> +
>>> +    size_t len_to_read = std::min(len, sizeof(msg) - 1);
>>> +
>>> +    read_all(&msg, len_to_read);
>>> +    msg.msg[len_to_read - sizeof(StreamMsgNotifyError)] = '\0';
>>> +
>>> +    syslog(LOG_ERR, "Received NotifyError message from the server: %d -
>>> %s\n",
>>> +        msg.error_code, msg.msg);
>>> +
>>> +    if (len_to_read < len) {
>>> +        throw std::runtime_error("Received NotifyError message size " +
>>> std::to_string(len) +
>>> +                                 " is too big (bigger than " +
>>> std::to_string(sizeof(msg)) + ")");
>>> +    }
>>> }
>>> 
>>> static void read_command_from_device(void)
> 
> Frediano