(smartcardio) Confusing bug fix for wrong DWORD typedef

Ivan Gerasimov ivan.gerasimov at oracle.com
Sat Jun 14 10:32:13 UTC 2014


Hi Yonathan!

Thank you for your close attention and analysis you've done.
I mostly agree with your conclusion that having correct typedefs for 
MacOSX would be a better solution here.
Even more radical approach would be to remove our own API declarations 
altogether and switch to the one provided with pcsc-lite.
That's what Ludovic Rousseau, the pcsc-lite author, has suggested in his 
blogpost [1].

If we ever decide to move this way, the problem you've mentioned will be 
gone along with many other problems of this kind.

With respect to the fix of 8043507, I've pushed, the solution was meant 
to be with the lowest possible risk of regression.
I only had to verify that it solves the problem on MacOSX.
Anything bigger might have required comprehensive testing across 
different platforms/hardware.

Sincerely yours,
Ivan

[1] 
http://ludovicrousseau.blogspot.co.uk/2013/03/oracle-javaxsmartcardio-failures.html



On 14.06.2014 3:45, Yonathan wrote:
> In a commit about a month ago[1], Ivan Gerasimov fixed a long-standing
> bug in using javax.smartcardio on OS X that I previously mentioned[2].
> I am uncomfortable with this fix, because to determine whether it was
> successful requires study of x86_64 calling conventions.
>
> Recall that the bug was that LPDWORD is typedef’d to unsigned long* in
> pcsc.c, but the dylib is actually expecting uint32_t*. The “fix” was
> to initialize variables with 0 before each function call. Instead, the
> fix should have been to typedef DWORD to uint32_t to match the library
> header. Consider this library declaration:
>
> LONG SCardStatus(SCARDHANDLE hCard,
>          LPTSTR mszReaderNames, LPDWORD pcchReaderLen,
>          LPDWORD pdwState,
>          LPDWORD pdwProtocol,
>          LPBYTE pbAtr, LPDWORD pcbAtrLen);
>
> When pcsc.c calls the function, does the function read/write the
> correct addresses? You have to think about the calling convention:
>
> hCard: %rdi → 8B stack variable; lib reads 4B little end
>
> mszReaderNames: %rsi → byte buffer on stack; lib read/writes bytes
>
> pcchReaderLen: %rdx → 8B stack variable; lib read/writes 4B little end
> unsigned and we interpret it as 8B long
>
> pdwState: %rcx → 8B stack variable; lib writes 4B little end bitmask
>
> pdwProtocol: %r8 → 8B stack variable; lib writes 4B little end enum
>
> pbAtr: %r9 → byte buffer on stack; lib writes bytes
>
> pcbAtrLen: 8B (%esp) → 8B stack variable; lib writes 4B little end and
> we interpret it as 8B long
>
> Because we assume x86_64 with 8-byte registers/8 byte parameter
> alignment and little-endian integers and all of them are unsigned, and
> the variables we write are bigger than the variables that the library
> reads, the fix should work, I think.
>
> But why not just typedef DWORD to match the typedef in the PCSC system
> header[3] on OS X so we don’t have to go through this analysis?
> Moreover, there is no comment in the code to state these assumptions,
> so this apparently redundant initialization is confusing.
>
> By the way, you can also simplify the related struct declaration
> fix[4] by using the same DWORD in it instead of declaring it in two
> different places.
>
> Thank you for your consideration.
>
> Yonathan
>
> [1] http://mail.openjdk.java.net/pipermail/security-dev/2014-May/010515.html
> [2] http://mail.openjdk.java.net/pipermail/security-dev/2013-March/006913.html
> [3] /System/Library/Frameworks/PCSC.framework/Headers/wintypes.h
> [4] http://mail.openjdk.java.net/pipermail/security-dev/2014-May/010498.html
>
>




More information about the security-dev mailing list