> On 7 Mar 2018, at 09:49, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> On Wed, 2018-03-07 at 01:53 -0500, Frediano Ziglio wrote:
>>>
>>>> On 3 Mar 2018, at 10:36, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>>>
>>>>>>
>>>>>> On 28 Feb 2018, at 13:53, Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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 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.
>>
>> 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.
>>
>>>>
>>>>> 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.
>
> Guys, this may be a stupid question,
There is no stupid question.
> but this kind of thing must be
> quite common, is there no industry standard/semi-standard way of doing
> it? Or at least a way it's done in SPICE?
There is no standard way of doing serialization in C++ due to lack of proper reflection information.
In the case of SPICE, the approach is in large part to use generated marshallers / demarshallers, which is one of the good ways to do it. The primary issue I know of at the moment from looking at it is that the code uses the data “in place” (i.e. it uses the data structures straight from the stream buffers), and I don’t know yet how to manager additional buffers if you need them for decoding (buffer lifetime being a bit difficult to manage, notably client-side, because you can have audio and video that are not in sync). Maybe someone else who knows better could give more details.
> At least that's what I
> expected you to tell me, like "yeah, just do it this way". So I didn't
> question what I was told and am now surprised by the amount of
> discussion about it...
>
> I can see this low-level serialization/deserialization code scattered
> everywhere, redoing the same thing.
For the streaming agent protocol itself, this is still very manual, we don’t use this kind of marshalling mechanism.
We also assume that host and guest have the same endianness, which is obviously always true on x86, but not, say, on Itanium.
> Why not factor it out into a
> library and use it everywhere (is it not possible?), or better use an
> existing one (this is most probably a rhetorical question, since the
> protocol is fixed and there probably isn't a library flexible enough to
> support it)?
>
> Cheers,
> Lukas
>
>>>>
>>>>>> +
>>>>>> + 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel