[jira] [Comment Edited] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.

["Alan Conway (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14739317#comment-14739317 ] 

Alan Conway edited comment on PROTON-992 at 9/10/15 6:36 PM:
-------------------------------------------------------------

OK, been looking at the cyrus sasl code. I believe that all we need to do is separate the per-process init/done logic and we are ok. From my scan it looks like any function of the form `sasl_*(sasl_conn_t...)` is safe to call if you serialize access on (at least) a per-connection basis, which all proton applications already do since proton itself requires serialization at least per-connection.

I believe when we separate the process wide init/shutdown code from the per-connection sasl code, all the dangerous `sasl_get_` functions will end up in the per-process code where they are not dangerous. The man page says that otherwise "most of libsasl is MT safe", which by my scan of the code means "it is safe to call sasl functions that take a sasl_conn_t parameter concurrently for different instances of sasl_conn_t. Nothing else should ever be called concurrently". 

I would completely ignore sasl_set_mutex, it looks like something somebody thought was a Good Idea but never actually implemented. The only place a mutex is actually locked is:

{code}
  /* serialize disposes. this is necessary because we can't
     dispose of conn->mutex if someone else is locked on it
     xxx there probably is a better way to do this */
  result = sasl_MUTEX_LOCK(dispose_mutex);
{code}

Hum, so we're using mutexes only to solve a problem created by the use of mutexes? My answer to "xxx" is yes, there is a better way: don't litter your code with mutexes that you don't even ever lock!!! There is no sasl_set_mutex man page in the latest source tarball so I'm guessing the folks at Cyrus realized it was a daft idea and are backing away, slowly.
As I read it, dispose is safe if you are serialized per connection.  Of course I could be wrong.


was (Author: aconway):
OK, been looking at the cyrus sasl code. I believe that all we need to do is separate the per-process init/done logic and we are ok. From my scan it looks like any function of the form `sasl_*(sasl_conn_t...)` is safe to call if you serialize access on (at least) a per-connection basis, which all proton applications already do since proton itself requires serialization at least per-connection.

I believe when we separate the process wide init/shutdown code from the per-connection sasl code, all the dangerous `sasl_get_` functions will end up in the per-process code where they are not dangerous. The man page says that otherwise "most of libsasl is MT safe", which by my scan of the code means "it is safe to call sasl functions that take a sasl_conn_t parameter concurrently for different instances of sasl_conn_t. Nothing else should ever be called concurrently". 

I would completely ignore sasl_set_mutex, it looks like something somebody thought was a Good Idea but never actually implemented. The only place a mutex is actually locked is:

{code}
  /* serialize disposes. this is necessary because we can't
     dispose of conn->mutex if someone else is locked on it
     xxx there probably is a better way to do this */
  result = sasl_MUTEX_LOCK(dispose_mutex);
{code}

Hum, so we're using mutexes only to solve a problem created by the use of mutexes? My answer to "xxx" is yes, there is a better way: don't litter your code with mutexes that you don't even ever lock!!! There is no sasl_set_mutex man page in the latest source tarball so I'm guessing the folks at Cyrus realized it was a daft idea and are backing away, slowly.
As I read it, dispose is safe if you are serialized per connection.  Of course I could be wrong, I have been before...
Of course I could be wrong.

> Proton's use of Cyrus SASL is not thread-safe.
> ----------------------------------------------
>
>                 Key: PROTON-992
>                 URL: https://issues.apache.org/jira/browse/PROTON-992
>             Project: Qpid Proton
>          Issue Type: Bug
>          Components: proton-c
>    Affects Versions: 0.10
>            Reporter: michael goulish
>            Assignee: michael goulish
>            Priority: Critical
>
> Documentation for the Cyrus SASL library says that the library is believed to be thread-safe only if the code that uses it meets several requirements.
> The requirements are:
>     * you supply mutex functions (see sasl_set_mutex())
>     * you make no libsasl calls until sasl_client/server_init() completes
>     * no libsasl calls are made after sasl_done() is begun
>     * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library.
> It says explicitly that that sasl_set* calls are not thread safe, since they set global state.
> The proton library makes calls to sasl_set* functions in :
>           pni_init_client()
>           pni_init_server(), and
>           pni_process_init()
> Since those are internal functions, there is no way for code that uses Proton to lock around those calls.
> I think proton needs a new API call to let applications call sasl_set_mutex().  Or something.
> We probably also need other protections to meet the other requirements specified in the Cyrus documentation (and quoted above).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)