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

[Frediano Ziglio]

> 
> > On 7 Mar 2018, at 09:49, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> > 
> > On Wed, 2018-03-07 at 01:53 -0500, Frediano Ziglio 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 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.
> 

This morning we tried to reuse the Python code but was a quite
unsuccessful attempt, format requires channel declarations and
generated code does different assumptions.

The marshalling/demarshalling generated can wither be in place
or not depends on structure declaration (in the .proto file)

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

This is partially correct. The protocol is defined little endian, the
server do the right conversions, the agent is assuming itself little
endian (which can be wrong).

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