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@xxxxxxxxxx>
> ---
> 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?
>
> 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;
> } @ctype(VSCMsgReaderAdd) reader_add = 101;
> +*/
Have you considered fixing the message numbering?
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel