[Pcsclite-muscle] pcsc_stringify_error thread safety

[Maksim Ivanov]

Ludovic,

Would you mind also extending the documentation for
pcsc_stringify_error to mention that:
1. The returned string is valid only while the thread on which is was
obtained is alive.
2. The returned string is valid until the next call to this function
on the same thread.


Thanks,
Maksim


On Wed, Jan 18, 2017 at 5:24 PM, Ludovic Rousseau
<ludovic.rousseau at gmail.com> wrote:
>
>
>
> 2017-01-18 14:41 GMT+01:00 Ludovic Rousseau <ludovic.rousseau at gmail.com>:
>>
>> Hello,
>>
>> 2017-01-18 11:29 GMT+01:00 Nikos Mavrogiannopoulos <nmav at redhat.com>:
>>>
>>> On Tue, 2017-01-17 at 20:33 +0100, Maksim Ivanov wrote:
>>>
>>> > The pcsc_stringify_error function in the PC/SC-Lite implementation
>>> > uses a statically allocated buffer. This means that the buffer may be
>>> > used simultaneously when the function is called from multiple threads
>>> > concurrently.
>>> > Therefore, the returned message may be spoiled, e.g.:
>>> > "Internal error.ul"
>>> > or
>>> > "Command cancell"
>>> > In the worst-case scenario, the application may read an unbounded
>>> > string (with the terminating null character missing).
>>>
>>> A possible fix is attached. That avoids copying strings which are
>>> constant on global store, and ensures that the static buffer is on
>>> thread local store when possible.
>>>
>>> Except compilation, the fix is completely untested.
>>
>>
>> A really simple fix is:
>> --- /var/folders/jb/2mvc64nx74b76qjg_5yk8zs00000gn/T//zsNKq9_error.c    2017-01-18 14:37:19.000000000 +0100
>> +++ src/error.c 2017-01-17 22:20:08.000000000 +0100
>> @@ -76,7 +76,7 @@ PCSC_API char* pcsc_stringify_error(cons
>>   */
>>  PCSC_API char* pcsc_stringify_error(const LONG pcscError)
>>  {
>> -   static char strError[75];
>> +   __thread static char strError[75];
>>     const char *msg = NULL;
>>
>>     switch (pcscError)
>>
>> I tested it with success.
>>
>> It looks like __thread is standard and not GNU C specific. So maybe your test to limit its use to GCC is not needed and, in fact, problematic.
>> According to https://en.wikipedia.org/wiki/Thread-local_storage it is supported by Solaris Studio C/C++, IBM XL C/C++, GNU C, Clang and Intel C++ Compiler (Linux systems).
>
>
> Fixed in https://github.com/LudovicRousseau/PCSC/commit/eab1d67295e4e1d5c12bbca77bc57c50fd384a4e
>
> Then the fix was fixed in https://github.com/LudovicRousseau/PCSC/commit/9726152f2c6767f0fa3103ad307dc28ef7923852
> Clang accepts "__thread static" but GCC does not and only accepts "static __thread".
>
>> Adding  const to the pcsc_stringify_error() protottype is also a good idea. I don't think it would break existing compilation. Any comment on that?
>
>
> Applied in https://github.com/LudovicRousseau/PCSC/commit/b27f0e54194dcfb4db8179dceede8a649141fea4
>
> Thansk to all the participants in this thread.
>
> Bye
>
> --
>  Dr. Ludovic Rousseau
>
> _______________________________________________
> Pcsclite-muscle mailing list
> Pcsclite-muscle at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle