[Spice-devel] [PATCH spice-common 1/4] Fix generation of Smartcard channel
Frediano Ziglio
fziglio at redhat.com
Wed May 16 15:16:55 UTC 2018
>
> On Mon, May 14, 2018 at 11:18:51PM +0100, Frediano Ziglio wrote:
> > The Smartcard channel definition has been always broken.
> > Multiple client messages with the same ID are defined in the channel.
> > This cause on server demarshaller to only have last message defined,
> > while on the client marshaller code all message marshallers are
> > defined but client uses only header message.
> >
> > Following the difference of the generated code.
> >
> > diff -rup old/generated_client_marshallers.c
> > common/generated_client_marshallers.c
> > --- old/generated_client_marshallers.c 2018-05-14 22:49:07.641778414
> > +0100
> > +++ common/generated_client_marshallers.c 2018-05-14 22:49:22.266329296
> > +0100
> > @@ -389,27 +389,6 @@ static void spice_marshall_msgc_tunnel_s
> > }
> >
> > #ifdef USE_SMARTCARD
> > -static void spice_marshall_msgc_smartcard_data(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED SpiceMsgcSmartcard *msg,
> > SpiceMarshaller **reader_name_out)
> > -{
> > - SPICE_GNUC_UNUSED SpiceMarshaller *m2;
> > - SpiceMsgcSmartcard *src;
> > - *reader_name_out = NULL;
> > - src = (SpiceMsgcSmartcard *)msg;
> > -
> > - /* header */ {
> > - spice_marshaller_add_uint32(m, src->header.type);
> > - spice_marshaller_add_uint32(m, src->header.reader_id);
> > - spice_marshaller_add_uint32(m, src->header.length);
> > - }
> > - if (src->header.type == SPICE_VSC_MESSAGE_TYPE_ReaderAdd) {
> > - /* Don't marshall @nomarshal reader_name */
> > - } else if (src->header.type == SPICE_VSC_MESSAGE_TYPE_ATR ||
> > src->header.type == SPICE_VSC_MESSAGE_TYPE_APDU) {
> > - /* Remaining data must be appended manually */
> > - } else if (src->header.type == SPICE_VSC_MESSAGE_TYPE_Error) {
> > - spice_marshaller_add_uint32(m, src->error.code);
> > - }
> > -}
> > -
> > static void spice_marshall_msgc_smartcard_header(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgHeader *msg)
> > {
> > SPICE_GNUC_UNUSED SpiceMarshaller *m2;
> > @@ -421,25 +400,6 @@ static void spice_marshall_msgc_smartcar
> > spice_marshaller_add_uint32(m, src->length);
> > }
> >
> > -static void spice_marshall_msgc_smartcard_error(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgError *msg)
> > -{
> > - SPICE_GNUC_UNUSED SpiceMarshaller *m2;
> > - VSCMsgError *src;
> > - src = (VSCMsgError *)msg;
> > -
> > - spice_marshaller_add_uint32(m, src->code);
> > -}
> > -
> > -static void spice_marshall_msgc_smartcard_atr(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgATR *msg)
> > -{
> > - SPICE_GNUC_UNUSED SpiceMarshaller *m2;
> > -}
> > -
> > -static void spice_marshall_msgc_smartcard_reader_add(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED VSCMsgReaderAdd *msg)
> > -{
> > - SPICE_GNUC_UNUSED SpiceMarshaller *m2;
> > -}
> > -
> > #endif /* USE_SMARTCARD */
> > static void spice_marshall_SpiceMsgCompressedData(SPICE_GNUC_UNUSED
> > SpiceMarshaller *m, SPICE_GNUC_UNUSED SpiceMsgCompressedData *msg)
> > {
> > @@ -496,20 +456,8 @@ SpiceMessageMarshallers * spice_message_
> > marshallers.msgc_record_mode = spice_marshall_msgc_record_mode;
> > marshallers.msgc_record_start_mark =
> > spice_marshall_msgc_record_start_mark;
> > #ifdef USE_SMARTCARD
> > - marshallers.msgc_smartcard_atr = spice_marshall_msgc_smartcard_atr;
> > -#endif /* USE_SMARTCARD */
> > -#ifdef USE_SMARTCARD
> > - marshallers.msgc_smartcard_data =
> > spice_marshall_msgc_smartcard_data;
> > -#endif /* USE_SMARTCARD */
> > -#ifdef USE_SMARTCARD
> > - marshallers.msgc_smartcard_error =
> > spice_marshall_msgc_smartcard_error;
> > -#endif /* USE_SMARTCARD */
> > -#ifdef USE_SMARTCARD
> > marshallers.msgc_smartcard_header =
> > spice_marshall_msgc_smartcard_header;
> > #endif /* USE_SMARTCARD */
> > -#ifdef USE_SMARTCARD
> > - marshallers.msgc_smartcard_reader_add =
> > spice_marshall_msgc_smartcard_reader_add;
> > -#endif /* USE_SMARTCARD */
> > marshallers.msgc_tunnel_service_add =
> > spice_marshall_msgc_tunnel_service_add;
> > marshallers.msgc_tunnel_service_remove =
> > spice_marshall_msgc_tunnel_service_remove;
> > marshallers.msgc_tunnel_socket_closed =
> > spice_marshall_msgc_tunnel_socket_closed;
> > diff -rup old/generated_client_marshallers.h
> > common/generated_client_marshallers.h
> > --- old/generated_client_marshallers.h 2018-05-14 22:49:07.641778414
> > +0100
> > +++ common/generated_client_marshallers.h 2018-05-14 22:49:22.739358627
> > +0100
> > @@ -61,11 +61,7 @@ typedef struct {
> > void (*msgc_tunnel_socket_data)(SpiceMarshaller *m,
> > SpiceMsgcTunnelSocketData *msg);
> > void (*msgc_tunnel_socket_token)(SpiceMarshaller *m,
> > SpiceMsgcTunnelSocketTokens *msg);
> > #ifdef USE_SMARTCARD
> > - void (*msgc_smartcard_data)(SpiceMarshaller *m, SpiceMsgcSmartcard
> > *msg, SpiceMarshaller **reader_name_out);
> > void (*msgc_smartcard_header)(SpiceMarshaller *m, VSCMsgHeader
> > *msg);
> > - void (*msgc_smartcard_error)(SpiceMarshaller *m, VSCMsgError *msg);
> > - void (*msgc_smartcard_atr)(SpiceMarshaller *m, VSCMsgATR *msg);
> > - void (*msgc_smartcard_reader_add)(SpiceMarshaller *m,
> > VSCMsgReaderAdd *msg);
> > #endif /* USE_SMARTCARD */
> > void (*msg_SpiceMsgCompressedData)(SpiceMarshaller *m,
> > SpiceMsgCompressedData *msg);
> > void (*msgc_port_event)(SpiceMarshaller *m, SpiceMsgcPortEvent
> > *msg);
> > Only in common/: generated_client_marshallers.lo
> > diff -rup old/generated_server_demarshallers.c
> > common/generated_server_demarshallers.c
> > --- old/generated_server_demarshallers.c 2018-05-14 22:49:07.641778414
> > +0100
> > +++ common/generated_server_demarshallers.c 2018-05-14 22:49:23.498405695
> > +0100
> > @@ -1957,24 +1957,18 @@ static uint8_t * parse_TunnelChannel_msg
> >
> > #ifdef USE_SMARTCARD
> >
> > -static uint8_t * parse_msgc_smartcard_reader_add(uint8_t *message_start,
> > uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size,
> > message_destructor_t *free_message)
> > +static uint8_t * parse_msgc_smartcard_header(uint8_t *message_start,
> > uint8_t *message_end, SPICE_GNUC_UNUSED int minor, size_t *size,
> > message_destructor_t *free_message)
> > {
> > SPICE_GNUC_UNUSED uint8_t *pos;
> > uint8_t *start = message_start;
> > uint8_t *data = NULL;
> > uint64_t nw_size;
> > + uint64_t mem_size;
> > uint8_t *in, *end;
> > - uint64_t reader_name__nw_size;
> > - uint64_t reader_name__nelements;
> > - VSCMsgReaderAdd *out;
> > + VSCMsgHeader *out;
> >
> > - { /* reader_name */
> > - reader_name__nelements = message_end - (start + 0);
> > -
> > - reader_name__nw_size = reader_name__nelements;
> > - }
> > -
> > - nw_size = 0 + reader_name__nw_size;
> > + nw_size = 12;
> > + mem_size = sizeof(VSCMsgHeader);
> >
> > /* Check if message fits in reported side */
> > if (nw_size > (uintptr_t) (message_end - start)) {
> > @@ -1986,13 +1980,14 @@ static uint8_t * parse_msgc_smartcard_re
> > if (SPICE_UNLIKELY(data == NULL)) {
> > goto error;
> > }
> > - end = data + sizeof(VSCMsgReaderAdd);
> > + end = data + sizeof(VSCMsgHeader);
> > in = start;
> >
> > - out = (VSCMsgReaderAdd *)data;
> > + out = (VSCMsgHeader *)data;
> >
> > - memcpy(out->reader_name, in, reader_name__nelements);
> > - in += reader_name__nelements;
> > + out->type = consume_uint32(&in);
> > + out->reader_id = consume_uint32(&in);
> > + out->length = consume_uint32(&in);
> >
> > assert(in <= message_end);
> > assert(end <= data + mem_size);
> > @@ -2017,7 +2012,7 @@ static uint8_t * parse_SmartcardChannel_
> > parse_msgc_disconnecting
> > };
> > static parse_msg_func_t funcs2[1] = {
> > - parse_msgc_smartcard_reader_add
> > + parse_msgc_smartcard_header
> > };
> > if (message_type >= 1 && message_type < 7) {
> > return funcs1[message_type-1](message_start, message_end, minor,
> > size_out, free_message);
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > python_modules/demarshal.py | 3 +--
> > spice.proto | 18 +++++++++++++-----
> > 2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index 8d3f5cb..7b53361 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -1039,8 +1039,7 @@ def write_msg_parser(writer, message):
> > msg_type = message.c_type()
> > msg_sizeof = message.sizeof()
> >
> > - want_mem_size = (len(message.members) != 1 or
> > message.members[0].is_fixed_nw_size()
> > - or not message.members[0].is_array())
> > + want_mem_size = not message.has_attr("nocopy")
>
> I believe this is not strictly required by this commit?
>
No, you are right, I'll remove from this.
> >
> > writer.newline()
> > if message.has_attr("ifdef"):
> > diff --git a/spice.proto b/spice.proto
> > index 4d916bb..802cb19 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -1383,11 +1383,11 @@ struct VscMessageAPDU {
> > } @ctype(VSCMsgAPDU);
> >
> > struct VscMessageATR {
> > - uint8 data[];
> > + uint8 atr[];
> > } @ctype(VSCMsgATR);
> >
> > struct VscMessageReaderAdd {
> > - int8 *reader_name[] @zero_terminated @nonnull @end @nomarshal;
> > + int8 name[] @zero_terminated @nonnull @end @nomarshal;
> > } @ctype(VSCMsgReaderAdd);
>
> Ditto for these 2 hunks which could be split
>
> >
> > channel SmartcardChannel : BaseChannel {
> > @@ -1400,6 +1400,12 @@ channel SmartcardChannel : BaseChannel {
> > } @ctype(SpiceMsgSmartcard) data = 101;
> >
> > client:
> > +/* Some of the following messages are duplicated, the protocol
> > + * definition was broken. Messages, as you can see have same ID.
> > + * Also code was not used and didn't compile correctly.
> > + * Keeping in the hope could be useful in the future.
> > + */
> > +/*
> > message {
> > VscMessageHeader header;
> > switch (header.type) {
> > @@ -1412,13 +1418,14 @@ channel SmartcardChannel : BaseChannel {
> > VscMessageError error;
> > } u @anon;
> > } @ctype(SpiceMsgcSmartcard) data = 101;
> > -
> > +*/
> > message {
> > vsc_message_type type;
> > uint32 reader_id;
> > uint32 length;
> > } @ctype(VSCMsgHeader) header = 101;
> > -
> > +/* See comment on client data message above */
> > +/*
> > message {
> > uint32 code;
> > } @ctype(VSCMsgError) error = 101;
> > @@ -1428,8 +1435,9 @@ channel SmartcardChannel : BaseChannel {
> > } @ctype(VSCMsgATR) atr = 101;
> >
> > message {
> > - int8 reader_name[] @zero_terminated @nonnull;
> > + int8 name[] @zero_terminated @nonnull;
Also this hunk can be removed (now in a comment)
> > } @ctype(VSCMsgReaderAdd) reader_add = 101;
> > +*/
>
> Have you considered fixing the message numbering?
>
Actually the numbering are kind of right, there's only a message...
In the server the parsing is done manually so the code generated from spice.proto
is not used (actually is not even compiled, as the hack in spice-common is doing).
In the client the marshalling is done using header message (the only one not
commented above). The python code currently accepts to have multiple messages with
same id and generate all marshallers but only the final demarshaller (yes, crazy)
so client in theory could use all marshallers.
The real message should be an union, kind of the "data" message, however this would
currently break the client code.
I tried to fix the "data" union message but had to use some weird syntax and
the generated code didn't seems totally correct so I decided to go to a simpler
but correct solution. Considered that the target is to fix the build removing
the hack and is currently not that easy to check I prefer this way.
> Christophe
>
Frediano
More information about the Spice-devel
mailing list