[Pcsclite-muscle] pcsc_stringify_error thread safety

[Ludovic Rousseau]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20170118/575d7d0e/attachment.html>