[Spice-devel] [PATCH spice-common] proto: Demarshal Smartcard data field

[Victor Toso]

Hi,

On Mon, Oct 07, 2019 at 11:38:58AM +0100, Frediano Ziglio wrote:
> Currently the demarshaler code is not used by spice-server.
> Demarshal all the fields of the header message, not only the header.
> Using generated code allows to easily check data and support
> big endian machines. Generated code will be used by spice-server.
> 
> The resulting change is.
> 
>    diff -ru gen/generated_client_marshallers.c common/generated_client_marshallers.c
>     --- gen/generated_client_marshallers.c      2019-10-05 20:44:54.000000000 +0100
>     +++ common/generated_client_marshallers.c   2019-10-05 20:45:33.000000000 +0100
>     @@ -283,6 +283,7 @@
>          spice_marshaller_add_uint32(m, src->type);
>          spice_marshaller_add_uint32(m, src->reader_id);
>          spice_marshaller_add_uint32(m, src->length);
>     +    /* Don't marshall @nomarshal data */
>      }
> 
>      #endif /* USE_SMARTCARD */
>     diff -ru gen/generated_server_demarshallers.c common/generated_server_demarshallers.c
>     --- gen/generated_server_demarshallers.c    2019-10-05 20:44:54.000000000 +0100
>     +++ common/generated_server_demarshallers.c 2019-10-05 20:45:33.000000000 +0100
>     @@ -1451,10 +1451,25 @@
>          uint64_t nw_size;
>          uint64_t mem_size;
>          uint8_t *in, *end;
>     +    uint64_t data__nw_size, data__mem_size;
>     +    uint64_t data__nelements;
>          VSCMsgHeader *out;
> 
>     -    nw_size = 12;
>     -    mem_size = sizeof(VSCMsgHeader);
>     +    { /* data */
>     +        uint32_t length__value;
>     +        pos = start + 8;
>     +        if (SPICE_UNLIKELY(pos + 4 > message_end)) {
>     +            goto error;
>     +        }
>     +        length__value = read_uint32(pos);
>     +        data__nelements = length__value;
>     +
>     +        data__nw_size = data__nelements;
>     +        data__mem_size = sizeof(uint8_t) * data__nelements;
>     +    }
>     +
>     +    nw_size = 12 + data__nw_size;
>     +    mem_size = sizeof(VSCMsgHeader) + data__mem_size;
> 
>          /* Check if message fits in reported side */
>          if (nw_size > (uintptr_t) (message_end - start)) {
>     @@ -1474,6 +1489,10 @@
>          out->type = consume_uint32(&in);
>          out->reader_id = consume_uint32(&in);
>          out->length = consume_uint32(&in);
>     +    verify(sizeof(out->data) == 0);
>     +    memcpy(out->data, in, data__nelements);
>     +    in += data__nelements;
>     +    end += data__nelements;
> 
>          assert(in <= message_end);
>          assert(end <= data + mem_size);
> 
> The @nomarshal attribute allows to not change the marshaling code
> (used by spice-gtk).

Ah, I was thinking about that for a bit. That's interesting case.
We do have data being sent but not described in the spice.proto
file. From protocol point of view, I found that very weird. Is
it? (honest question)

> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Patch seems fine, I've asked before elsewhere but I'd love to
have a simple way to test smartcard if that exists!

> ---
>  spice.proto | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/spice.proto b/spice.proto
> index 34ba3c8..616b960 100644
> --- a/spice.proto
> +++ b/spice.proto
> @@ -1305,6 +1305,7 @@ channel SmartcardChannel : BaseChannel {
>          vsc_message_type type;
>          uint32 reader_id;
>          uint32 length;
> +        uint8 data[length] @end @nomarshal;

This truly seems like a protocol fix to me.

Cheers,

>      } @ctype(VSCMsgHeader) header = 101;
>  /* See comment on client data message above */
>  /*
> -- 
> 2.21.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20191007/739c4eea/attachment.sig>