Re: [Qemu-devel] [PATCH 4/7] libcacard: initial commit

[Alon Levy]

On Mon, Mar 14, 2011 at 04:20:22PM +0100, Jes Sorensen wrote:
> On 02/23/11 12:20, Alon Levy wrote:
> > +/* private data for PKI applets */
> > +typedef struct CACPKIAppletDataStruct {
> > +    unsigned char *cert;
> > +    int cert_len;
> > +    unsigned char *cert_buffer;
> > +    int cert_buffer_len;
> > +    unsigned char *sign_buffer;
> > +    int sign_buffer_len;
> > +    VCardKey *key;
> > +} CACPKIAppletData;
> 
> Grouping the ints together would allow for better struct padding.
> 
> > +/*
> > + *  resest the inter call state between applet selects
> > + */
> 
> I presume it meant to say 'resets' ?
> 
> > diff --git a/libcacard/event.c b/libcacard/event.c
> > new file mode 100644
> > index 0000000..20276a0
> > --- /dev/null
> > +++ b/libcacard/event.c
> > @@ -0,0 +1,112 @@
> > +/*
> > + *
> > + */
> 
> This comment is really spot on :)
> 
> > diff --git a/libcacard/mutex.h b/libcacard/mutex.h
> > new file mode 100644
> > index 0000000..cb46aa7
> > --- /dev/null
> > +++ b/libcacard/mutex.h
> 
> UH OH!
> 
> > +/*
> > + *  This header file provides a way of mapping windows and linux thread 
> > calls
> > + *  to a set of macros.  Ideally this would be shared by whatever 
> > subsystem we
> > + *  link with.
> > + */
> > +
> > +#ifndef _H_MUTEX
> > +#define _H_MUTEX
> > +#ifdef _WIN32
> > +#include <windows.h>
> > +typedef CRITICAL_SECTION mutex_t;
> > +#define MUTEX_INIT(mutex) InitializeCriticalSection(&mutex)
> > +#define MUTEX_LOCK(mutex) EnterCriticalSection(&mutex)
> > +#define MUTEX_UNLOCK(mutex) LeaveCriticalSection(&mutex)
> > +typedef CONDITION_VARIABLE condition_t;
> > +#define CONDITION_INIT(cond) InitializeConditionVariable(&cond)
> > +#define CONDITION_WAIT(cond, mutex) \
> > +            SleepConditionVariableCS(&cond, &mutex, INFINTE)
> > +#define CONDITION_NOTIFY(cond) WakeConditionVariable(&cond)
> > +typedef uint32_t thread_t;
> > +typedef HANDLE thread_status_t;
> > +#define THREAD_CREATE(tid, func, arg) \
> > +        CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)func, arg, 0, &tid)
> > +#define THREAD_SUCCESS(status) ((status) !=  NULL)
> > +#else
> > +#include <pthread.h>
> > +typedef pthread_mutex_t mutex_t;
> > +#define MUTEX_INIT(mutex) pthread_mutex_init(&mutex, NULL)
> > +#define MUTEX_LOCK(mutex) pthread_mutex_lock(&mutex)
> > +#define MUTEX_UNLOCK(mutex) pthread_mutex_unlock(&mutex)
> > +typedef pthread_cond_t condition_t;
> > +#define CONDITION_INIT(cond) pthread_cond_init(&cond, NULL)
> > +#define CONDITION_WAIT(cond, mutex) pthread_cond_wait(&cond, &mutex)
> > +#define CONDITION_NOTIFY(cond) pthread_cond_signal(&cond)
> > +typedef pthread_t thread_t;
> > +typedef int thread_status_t;
> > +#define THREAD_CREATE(tid, func, arg) pthread_create(&tid, NULL, func, arg)
> > +#define THREAD_SUCCESS(status)  ((status) == 0)
> > +#endif
> 
> NACK! This is no good, the code needs to be fixed to use QemuMutex from
> qemu-thread.h
> 
> In addition, a thing like a mutex feature should be in a separate patch,
>  not part of the code that uses it. However QEMU already has a mutex set
> so this part needs to go.
> 
> > +static VCardAppletPrivate *
> > +passthru_new_applet_private(VReader *reader)
> > +{
> > +    VCardAppletPrivate *applet_private = NULL;
> > +    LONG rv;
> > +
> > +    applet_private = (VCardAppletPrivate 
> > *)malloc(sizeof(VCardAppletPrivate));
> 
> qemu_malloc()
> 
> > +    if (applet_private == NULL) {
> > +        goto fail;
> > +    }
> 
> and it never fails.
> 
> > +        if (new_reader_list_len != reader_list_len) {
> > +            /* update the list */
> > +            new_reader_list = (char *)malloc(new_reader_list_len);
> 
> qemu_malloc() again.
> 
> Please grep through the full patch set and make sure you do not have any
> direct calls to malloc() or free().
> 
> > +            /* try resetting the pcsc_lite subsystem */
> > +            SCardReleaseContext(global_context);
> > +            global_context = 0; /* should close it */
> > +            printf("***** SCard failure %x\n", rv);
> 
> error_report()
> 
> > diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> > new file mode 100644
> > index 0000000..e4f0b73
> > --- /dev/null
> > +++ b/libcacard/vcard_emul_nss.c
> [snip]
> > +struct VReaderEmulStruct {
> > +    PK11SlotInfo *slot;
> > +    VCardEmulType default_type;
> > +    char *type_params;
> > +    PRBool present;
> 
> What is PRBool and where does it come from?
> 
> > +void
> > +vcard_emul_reset(VCard *card, VCardPower power)
> > +{
> > +    PK11SlotInfo *slot;
> > +
> > +    if (!nss_emul_init) {
> > +        return;
> > +    }
> > +
> > +    /* if we reset the card (either power on or power off), we loose our 
> > login
> > +     * state */
> > +    /* TODO: we may also need to send insertion/removal events? */
> > +    slot = vcard_emul_card_get_slot(card);
> > +    (void)PK11_Logout(slot);
> 
> We don't (void) cast calls like that in QEMU.
> 
> > +/*
> > + *  NSS needs the app to supply a password prompt. In our case the only 
> > time
> > + *  the password is supplied is as part of the Login APDU. The actual 
> > password
> > + *  is passed in the pw_arg in that case. In all other cases pw_arg should 
> > be
> > + *  NULL.
> > + */
> > +static char *
> > +vcard_emul_get_password(PK11SlotInfo *slot, PRBool retries, void *pw_arg)
> > +{
> > +    /* if it didn't work the first time, don't keep trying */
> > +    if (retries) {
> > +        return NULL;
> > +    }
> > +    /* we are looking up a password when we don't have one in hand */
> > +    if (pw_arg == NULL) {
> > +        return NULL;
> > +    }
> > +    /* TODO: we really should verify that were are using the right slot */
> > +    return PORT_Strdup(pw_arg);
> 
> PORT_Strdup???
> 
> > +static const char *
> > +find_token(const char *str, char token, char token_end)
> > +{
> > +    /* just do the blind simple thing */
> > +    for (; *str; str++) {
> > +        if ((*str == token) || (*str == token_end)) {
> > +            break;
> > +        }
> > +    }
> > +    return str;
> > +}
> 
> What is wrong with strpbrk(3) ?
> 
> > +static const char *
> > +strip(const char *str)
> > +{
> > +    for (; *str; str++) {
> > +        if ((*str != ' ') && (*str != '\n') &&
> > +           (*str != '\t') && (*str != '\r')) {
> > +            break;
> 
> !isspace() ?
> 
> > +static const char *
> > +find_blank(const char *str)
> > +{
> > +    for (; *str; str++) {
> > +        if ((*str == ' ') || (*str == '\n') ||
> > +           (*str == '\t') || (*str == '\r')) {
> > +
> > +            break;
> > +        }
> > +    }
> > +    return str;
> > +}
> 
> strpbrk(3) ?
> 
> > diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
> > new file mode 100644
> > index 0000000..8bca16e
> > --- /dev/null
> > +++ b/libcacard/vcardt.h
> > @@ -0,0 +1,66 @@
> > +/*
> > + *
> > + */
> > +#ifndef VCARDT_H
> > +#define VCARDT_H 1
> > +
> > +/*
> > + * these should come from some common spice header file
> > + */
> > +#include <assert.h>
> > +#ifndef ASSERT
> > +#define ASSERT assert
> > +#endif
> > +#ifndef MIN
> > +#define MIN(x, y) ((x) > (y) ? (y) : (x))
> > +#define MAX(x, y) ((x) > (y) ? (x) : (y))
> > +#endif
> 
> QEMU uses assert(), not ASSERT(). Please fix.
> 
> > diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> > new file mode 100644
> > index 0000000..f4a0c60
> > --- /dev/null
> > +++ b/libcacard/vreader.c
> > @@ -0,0 +1,526 @@
> > +/*
> > + * emulate the reader
> > + */
> 
> What is the license of this file?
> 
> > +#include "vcard.h"
> > +#include "vcard_emul.h"
> > +#include "card_7816.h"
> > +#include "vreader.h"
> > +#include "vevent.h"
> > +
> > +/*
> > + * System includes
> > + */
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +/*
> > + * spice includes
> > + */
> > +#include "mutex.h"
> 
> No no no
> 
> > diff --git a/libcacard/vreadert.h b/libcacard/vreadert.h
> > new file mode 100644
> > index 0000000..51670a3
> > --- /dev/null
> > +++ b/libcacard/vreadert.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + *
> > + */
> 
> Spot on!
> 
> General comment, this patch is *way* too big. It really should be a
> series of patches adding features one after another. The testing for
> cards ought to be separate for example.
> 

I've got everything done except the split. Is there any value in sending
it without doing the split? could you give me some hints. As I mentioned
off line, this code was not written by me, which makes this harder (but
not impossible)

Alon

> Jes
>