(smartcardio) Confusing bug fix for wrong DWORD typedef (original) (raw)

Yonathan yonathan at gmail.com
Sat Jun 14 17:41:34 UTC 2014

Thanks Ivan. The current solution does make sense if the cleaner solution would trigger difficult testing. And I do think the solution works, as you have initialized all DWORDs before calls.

Cheers, Yonathan

On Sat, Jun 14, 2014 at 3:32 AM, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:

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 x8664 calling conventions. Recall that the bug was that LPDWORD is typedef’d to unsigned long* in pcsc.c, but the dylib is actually expecting uint32t*. The “fix” was to initialize variables with 0 before each function call. Instead, the fix should have been to typedef DWORD to uint32t 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 x8664 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