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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel