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.gerasi...@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 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 >> >> >
