(smartcardio) Confusing bug fix for wrong DWORD typedef

[Yonathan]

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 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
>>
>>
>