[Spice-devel] [PATCH spice-streaming-agent v5 2/2] Implement handling of error messages from the server
Christophe de Dinechin
cdupontd at redhat.com
Wed Mar 7 09:53:00 UTC 2018
> On 7 Mar 2018, at 07:53, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>>
>>> 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.
>>
>
> Yes, can be done (I'd suggest skipping the rest).
> Currently errors are fatal.
>
>>>
>>>>> + } 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
>>
>
> Some compiler flag are used to break the ABI
I used the compiler flag only to illustrate that relying on the absence of padding was not portable.
Did you really miss that my point was: “not portable” and not “hey, let’s use mysterious flags for the sake of it"?
> and should be used in specific cases like some optimized code or writing a language library
> if the language for some reason requires a different ABI, you won't
> be able to call libc function safely if you use such options and as
> you want to break the ABI you'll get an ABI breakage.
Is a lecture on ABI breaking flags entirely necessary here?
I used one to prove that the code is ABI-dependent, therefore not portable.
>
> My suggestion on overriding the field was not intended.
>
>>
>>>> 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?
>>
>
> Already replied.
I remember only one response from you, which was "Try to see why ip protocol take into account alignment.” (2018-02-22). That left me to guess what you meant, since there was no link or explanation. I’m also puzzled by this response, since I’m pretty sure the IP protocol was invented long before the “packed” attribute.
If you imply that packed is generally not used in network protocol, I believe this is false, or at least that the Linux kernel did not get the memo. For instance:
/* common bits */
struct ceph_x_ticket_blob {
__u8 struct_v;
__le64 secret_id;
__le32 blob_len;
char blob[];
} __attribute__ ((packed));
Now, that structure definition I understand quite well. It’s packed, and it specifies that the data is little endian.
So would you please be kind enough to elaborate why you believe there should be no packed attribute in the SPICE protocol, or refer me to the message where there is such an explanation (I obviously missed it, and I searched)?
>
>>>
>>>> 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.
>>
>
> Yes, override was not intended.
> Unnamed structure are quite common, really used daily but a name won't surely hurt.
Anonymous structs are a relatively frequent way of doing things in C, yes. But not in C++.
And here, it’s not just anonymous, it’s an anonymous derived struct. Honestly, that’s the first time I see that. Can you show me another example?
>
>>>
>>>>> +
>>>>> + 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list