[Spice-devel] [xf86-video-qxl v5] Enable smartcard support for XSpice.

[Jeremy White]

On 12/12/2014 12:16 PM, Marc-André Lureau wrote:
> The spice bits looks good to me, the pcsc ccid module builds fine, and
> looks ok except the read loop (see below) and I haven't tried it. I
> would have added a couple more pre-conditions, for example if the
> client sends VSC_ReaderAdd several times without VCS_ReaderRemove I
> guess it crashes or leaks, perhaps this could be handled a bit better?

Yes, I guess that's true.  The calls are generally all triggered by 
libraries that govern behavior; I think it would be odd to get malicious 
patterns like that.  But I think it's easy to address.

I found two issues:  we could call pthread_mutex_init repeatedly on a 
ReaderAdd/ReaderAdd pattern, and we'd call pthread_mutex_lock on an 
uninitialied lock in a VCS_ReaderAdd/VCS_ReaderRemove/VCS_APDU sequence.

Did you see anything else I missed?

>
>
>> +static void process_atr(smartcard_ccid_t *ccid, VSCMsgHeader *h, char *data)
>> +{
>> +    ccid->atr_len = MIN(h->length, sizeof(ccid->atr));
>> +
>
> ATR length truncation could use a warning,

Sure; can't hurt anything.

>> +    while (1) {
>> +        rc = read(ccid->fd, buf + pos, sizeof(buf) - pos);
>> +        if (rc == -1)
>> +            if (errno == EINTR)
>> +                continue;
>> +            else
>> +                break;
>> +
>> +        if (rc == 0)
>> +            break;
>> +
>> +        pos += rc;
>> +
>> +        do {
>> +            rc = process_message(ccid, buf, pos);
>> +            pos -= rc;
>
> So you assume that messages are not sent "in batch" and you won't
> override previous message? I guess you could assert() that pos == rc.
> And then this loop is useless right? Or you need to increment buf?
> hmm, I think you should rather memcpy the remaining bits if any
>
>> +        } while (rc > 0);
>
> I guess it could warn if pos == sizeof(buf) here

Thanks for assuming the best of me, but it's just a bug on my part :-/.

I believe adding this to the bottom of the loop:
             if (rc > 0 && pos > 0)
                 memmove(buf, buf + rc, pos);

corrects it.

I'll send a v6 in a bit; thanks for the review.

Cheers,

Jeremy