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

[Frediano Ziglio]

> 
> 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)
> 

There are quite a lot of oddities in the Smartcard specifically:
- server does not use parser (removed by next series)
- multiple messages with same ids (removed, still in comments)
- structures from external header(s) used (instead of just spice)

It's weird, some of "@nomarshal" are used for ending arrays to
avoid a potential extra copy. But at least @nomarshal fields
are in the protocol.

It's actually a bit uncommon that the .proto files contains some
details of the specific client/server implementations.

But surely would be nice if the protocol messages describe the
full message with at least some raw data is the embedded data
are "external" (coming from other structures or marshallers
for instance).

It's also odd that the network structures are little endian
but the device ones are big endian (so you need to revert
moving from channel to device and vice versa).

About the test see patch 2/8. It's surely not full but it's
testing this path.

> > 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 */
> >  /*

Frediano