(smartcardio) Confusing bug fix for wrong DWORD typedef
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.
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 .
> 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
> Sincerely yours,
> On 14.06.2014 3:45, Yonathan wrote:
>> In a commit about a month ago, Ivan Gerasimov fixed a long-standing
>> bug in using javax.smartcardio on OS X that I previously mentioned.
>> 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 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 by using the same DWORD in it instead of declaring it in two
>> different places.
>> Thank you for your consideration.
>>  /System/Library/Frameworks/PCSC.framework/Headers/wintypes.h
More information about the security-dev