> On 28 Feb 2018, at 13:53, Lukáš Hrázký <lhra...@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ý <lhra...@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.

> +    } 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?

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

Please fix it.

> +
> +    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)
> -- 
> 2.16.1
> 
> _______________________________________________
> 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

Reply via email to