[Spice-devel] [PATCH] reds: send the number of channel in the LE byte-order

[Christophe Fergeau]

On Fri, Mar 27, 2015 at 05:49:30PM +0300, Denis Kirjanov wrote:
> On 3/27/15, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > Hey,
> >
> > On Thu, Mar 26, 2015 at 05:52:05PM +0300, Denis Kirjanov wrote:
> >> Since the spice protocol is LE send the number of channels in
> >> the proper byte order
> >>
> >> Signed-off-by: Denis Kirjanov <kda at linux-powerpc.org>
> >> ---
> >>  server/reds.c |    4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/server/reds.c b/server/reds.c
> >> index f61d5d3..468ac9c 100644
> >> --- a/server/reds.c
> >> +++ b/server/reds.c
> >> @@ -37,6 +37,8 @@
> >>  #include <ctype.h>
> >>  #include <stdbool.h>
> >>
> >> +#include <glib.h>
> >> +
> >>  #include <openssl/err.h>
> >>
> >>  #if HAVE_SASL
> >> @@ -874,7 +876,7 @@ void reds_fill_channels(SpiceMsgChannels
> >> *channels_info)
> >>          used_channels++;
> >>      }
> >>
> >> -    channels_info->num_of_channels = used_channels;
> >> +    channels_info->num_of_channels = GUINT32_TO_LE(used_channels);
> >
> > Are you sure this is needed? In other words, what concrete bug are you
> > observing without that change?
> > reds_fill_channels is called from main_channel_marshall_channels(),
> > which then passes the filled SpiceMsgChannels instance to
> > spice_marshall_msg_main_channels_list()
> >
> > spice_marshall_msg_* is generated from the description of the
> > SPICE protocol available in spice.proto/spice1.proto. These generated
> > marshallers are usually correct from an endianness point of view.
> >
> > This specific function is defined in
> > spice-common/common/generated_server_marshallers.c and does:
> >
> > spice_marshaller_add_uint32(m, src->num_of_channels);
> >
> > spice_marshaller_add_uint32() calls spice_marshaller_add_uint32() which
> > is
> > #ifdef WORDS_BIGENDIAN
> > #define write_uint32(ptr,v) (*((uint32_t *)(ptr)) =
> > SPICE_BYTESWAP32((uint32_t)(v)))
> > #else
> > #define write_uint32(ptr,v) (*((uint32_t *)(ptr)) = v)
> > #endif
> >
> > so I would say the existing code is correct, and this patch unneeded.
> 
> The problem is that write_uint32t writes the value in the BE even if
> the client is LE. Obviously it's wrong.

As I explained above, it's supposed to write it on the wire as a
little-endian number, so if you get a number of channels in big-endian
on the wire, something is wrong with the reasoning above. In this case
it would be good to understand which part of it is wrong, as a different
fix may be needed.
If you get a big endian value, one thing which could go wrong is
WORDS_BIGENDIAN not being defined when marshaller.c is compiled on a BE
machine. I'm sure more things could fail :)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150327/d19a8c66/attachment.sig>