pcsc_stringify_error thread safety

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

pcsc_stringify_error thread safety

Maksim Ivanov
Hello,

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


One possible solution would be to return addresses derived from the
string literals directly.
However, this won't support the dynamic message formatting, which is
currently performed for unknown error codes (though this arguably
doesn't look like a vital feature).


Regards,
Maksim

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Ivo Raisr


On 17.1.2017 20:33, Maksim Ivanov wrote:
> One possible solution would be to return addresses derived from the
> string literals directly.
> However, this won't support the dynamic message formatting, which is
> currently performed for unknown error codes (though this arguably
> doesn't look like a vital feature).
Another approach is thread-specific data (TSD).
I.


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Nikos Mavrogiannopoulos
In reply to this post by Maksim Ivanov
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.

regards,
Nikos

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

0001-pcsc_stringify_error-address-overlapping-static-vari.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Ludovic Rousseau
Hello,

2017-01-18 11:29 GMT+01:00 Nikos Mavrogiannopoulos <[hidden email]>:
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).

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?

Bye

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Ludovic Rousseau


2017-01-18 14:41 GMT+01:00 Ludovic Rousseau <[hidden email]>:
Hello,

2017-01-18 11:29 GMT+01:00 Nikos Mavrogiannopoulos <[hidden email]>:
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).


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?

Thansk to all the participants in this thread.

Bye

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: 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
<[hidden email]> wrote:

>
>
>
> 2017-01-18 14:41 GMT+01:00 Ludovic Rousseau <[hidden email]>:
>>
>> Hello,
>>
>> 2017-01-18 11:29 GMT+01:00 Nikos Mavrogiannopoulos <[hidden email]>:
>>>
>>> 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
> [hidden email]
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Ludovic Rousseau


2017-01-18 18:42 GMT+01:00 Maksim Ivanov <[hidden email]>:
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

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: pcsc_stringify_error thread safety

Nikos Mavrogiannopoulos
In reply to this post by Ludovic Rousseau
----- Original Message -----

> Hello,
>
> 2017-01-18 11:29 GMT+01:00 Nikos Mavrogiannopoulos <[hidden email]>:
>
> > 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.

A bit late, however, as far as I know the C11 standard defines _Thread_local
and not __thread. The latter is supported by some compilers, but it is not
in any standard I know of. In any case, for most systems pcsc-lite will be
compiled on __thread is also fine.

regards,
Nikos

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle