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

[Uri Lublin]

On 03/04/2018 12:38 PM, Uri Lublin wrote:
> On 02/28/2018 02:53 PM, Lukáš Hrázký 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 {
>> +        uint8_t msg[1024];
>> +    } msg;
>> +
>> +    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';
> 
> That looks wrong. It should be:
>         msg.msg[len_to_read] = '\0';
> Otherwise, the end of the message (last 4 bytes) is cut out.
> 
> 
> After re-reading it, I realize there are two meanings to msg.msg:
> one from the definition above and one inherited.

After re-re-reading it, it seems the code is correct but confusing.
The local variable msg.msg and the inherited msg.msg point to the
same location in memory (after StreamMsgNotifyError.error_code).

Uri.

> 
> Uri.
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel