>
> > On 3 Mar 2018, at 10:36, Frediano Ziglio <fzig...@redhat.com> wrote:
> >
> >>>
> >>> On 28 Feb 2018, at 13:53, Lukáš Hrázký <lhra...@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ý <lhra...@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.
> >
> >>> +
> >>> + 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
