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