winscard

[Vincent Hardy]

Hi all,

Here are four patches that are a partial but functional implementation 
of winscard.

I installed "Belgian Electronic Identity Card MiddleWare" 
(http://eid-mw.googlecode.com/files/BeidMW35-6995.msi)

I tested winscard by starting beid35gui.exe and I can see my identity 
card data.

Moreover, this should help to solve this bug 
http://bugs.winehq.org/show_bug.cgi?id=26978.

Are these 4 patches good enough to be sent to wine-patch ?

winscard is really useful for me and probably for other people too.

Vincent

[Bruno Jesus]

On Fri, Sep 30, 2011 at 06:42, Vincent Hardy  wrote:
> Hi all,
>
> Here are four patches that are a partial but functional implementation of
> winscard.

Congratulations =)
> I installed "Belgian Electronic Identity Card MiddleWare"
> (http://eid-mw.googlecode.com/files/BeidMW35-6995.msi)
>
> I tested winscard by starting beid35gui.exe and I can see my identity card
> data.

I can help you test this (although currently I'm lacking the time to),
I have 8 different contact and contactless readers and dozens of
different card types. And several demo applications and full fledged
hsm applications that run with over 200 smartcards at the same time =)
> Moreover, this should help to solve this bug
> http://bugs.winehq.org/show_bug.cgi?id=26978.
>
> Are these 4 patches good enough to be sent to wine-patch ?
>
> winscard is really useful for me and probably for other people too.

I'm not used to patch reviewing but here are my comments:

You don't need to check for invalid parameters on the functions, at
least in most of them. pcsclite should do that by itself and for
example in the SCardListreaders you don't need to interpret
AUTOALLOCATE because pcsclite handles that [1].

In SCardConnect you are also checking and returning the invalid
parameters error but pcsclite checks that [1].

Again with SCardStatus, I guess it should be as simple as a forward to pcsclite.

You're defining MAX_ATR_SIZE 36 but the standard ISO 7816 value is 33
bytes and both pcsclite and windows winscard use 33.

At least once you do HeapAlloc + memset zero, usually this can be done
by passing a flag HEAP_ZERO_MEMORY to HeapAlloc.
> Vincent

[1] = http://pcsclite.alioth.debian.org/api/winscard__clnt_8c_source.html

Best wishes,
Bruno

[Jeff Latimer]

On 30/09/11 19:42, Vincent Hardy wrote:
> Are these 4 patches good enough to be sent to wine-patch ?

Vincent, normally you would start by adding tests that can confirm the
same behaviour in Windows.  If you can do that and get the test accepted
then you are a long closer to get your patches accepted too.  As you
start with the tests you need to code the tests for functions you have
not implemented in the same patch as a todo wine so that the test will
not break the wine test suite.

[Marcus Meissner]

On Fri, Sep 30, 2011 at 11:42:26AM +0200, Vincent Hardy wrote:
> Hi all,
>
> Here are four patches that are a partial but functional
> implementation of winscard.
>
> I installed "Belgian Electronic Identity Card MiddleWare"
> (http://eid-mw.googlecode.com/files/BeidMW35-6995.msi)
>
> I tested winscard by starting beid35gui.exe and I can see my
> identity card data.
>
> Moreover, this should help to solve this bug
> http://bugs.winehq.org/show_bug.cgi?id=26978.
>
> Are these 4 patches good enough to be sent to wine-patch ?

Patch looks largely good to me. 1 comment I have on quick review below.

It is probably hard to write testcases that work without hardware,
but if you can think of any, they would be welcome.

....
> -
>                State = pStates[i].dwEventState & (~SCARD_STATE_CHANGED);
> -
>                rgReaderStates[i].cbAtr = pStates[i].cbAtr;
> -
>                memcpy(rgReaderStates[i].rgbAtr, pStates[i].rgbAtr, MAX_ATR_SIZE);

The "MAX_ATR_SIZE" size looks wrong.  Should it be perhaps pStates[i].cbAtr
which seems to be the bytecount?

With that clarified you could already send them to wine-patches as-is.

Ciao, Marcus

[Mounir IDRASSI]

On 9/30/2011 4:47 PM, Marcus Meissner wrote:
> > -
> >                State = pStates[i].dwEventState&  (~SCARD_STATE_CHANGED);
> > > -
> > >                rgReaderStates[i].cbAtr = pStates[i].cbAtr;
> > > -
> > >                memcpy(rgReaderStates[i].rgbAtr, pStates[i].rgbAtr, MAX_ATR_SIZE);
>
> The "MAX_ATR_SIZE" size looks wrong.  Should it be perhaps pStates[i].cbAtr
> which seems to be the bytecount?

I'll answer this since these patches are taken from my original big 
patch: copying cbAtr bytes makes more sens but copying MAX_ATR_SIZE is 
not wrong since the two buffers are of the same size and here we are 
just transferring all bytes from pcsclite side to the WIN32 side.
That being said, maybe putting pStates[i].cbAtr will help the patches be 
accepted.

By the way, I want to thank Vincent for taking the time to try to 
incorporate the winscard code into Wine. Without his dedication, this 
could languish for many years more!

Cheers,
--
Mounir IDRASSI
IDRIX
http://www.idrix.fr