[Spice-devel] [PATCH spice-streaming-agent v5 2/2] Implement handling of error messages from the server
Frediano Ziglio
fziglio at redhat.com
Wed Mar 7 10:19:55 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"?
>
Yes, also assuming uint8_t is not strictly portable.
Currently there's no real ABI that breaks either thing.
> > 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.
>
No, there are another reply on this ML, don't remember the email.
> 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?
>
As I said not against adding a name if is more readable for you.
Do you have any suggestion?
>
> >
> >>>
> >>>>> +
> >>>>> + 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
More information about the Spice-devel
mailing list