[jira] [Created] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
michael goulish created PROTON-992: -- Summary: 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 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)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14739069#comment-14739069 ] Alan Conway commented on PROTON-992: The general proton philosophy on thread safety is to be "thread agnosic". Calls involving values associated with a single engine or reactor *cannot* be made concurrently, but objects belonging to different engines or reactors are independent and can be used concurrently simply because there are no shared variables. Locking or serialization is left to higher layers, which is as it should be. We need to preserve that philosophy. The main problem is that proton tries to hide the sasl_client_init() and sasl_client_done() calls in calls that can be made per-connection. Looking at the cyrus code, these calls they are clearly not intended to be safe to call multiple times or concurrently as they over-write global state. Many C libraries have init/done functions that must be called exactly once per process (from main() in C, from static ctor/dtor in C++ etc.) It is a hateful requirement and proton has done well to avoid it but if we depend on external libraries I fear we can avoid it no more. We need global init/done functions at least for the sasl stuff. In a single threaded reactor we could call them as a convenience in default handlers for reactor start/stop events but code using multiple reactors in multiple thread contexts needs to be able to override such behavior, and real C programmers will prefer to know that all the global init/done functions are called in order under their control in main(). Practical experience with qpid C++ broker suggests that this is enough and after that per-connection serialization is sufficient, qpidd does not set the SASL mutex functions. However the SASL docs say it is *not* sufficient, and maybe qpidd is just lucky or a future version of SASL will break it. We should dig deeper before deciding what further serialization horrors need to be inflicted on the per-connection part of the SASL negotiation. > 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 >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)
[jira] [Comment Edited] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[
https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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
>
[jira] [Comment Edited] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[
https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14739317#comment-14739317
]
Alan Conway edited comment on PROTON-992 at 9/10/15 6:39 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, but this view is backed up by the qpidd c++
experience which is using SASL with per-process init and serialized per
connection with no incident so far, that we know of, probably.
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.
> 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.
[GitHub] qpid-proton pull request: PROTON-984: Document proton-j time units
Github user boo closed the pull request at: https://github.com/apache/qpid-proton/pull/50 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: Proton 980
Github user boo commented on the pull request: https://github.com/apache/qpid-proton/pull/49#issuecomment-139454327 rebased to master --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] qpid-proton pull request: Proton 980
Github user boo closed the pull request at: https://github.com/apache/qpid-proton/pull/49 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (PROTON-980) Enable handler processing the event after child handlers have processed it
[ https://issues.apache.org/jira/browse/PROTON-980?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14739484#comment-14739484 ] ASF subversion and git services commented on PROTON-980: Commit c8c2775b58e9a9be51c091cc56980cf344ce8cd1 in qpid-proton's branch refs/heads/master from [~bozzo] [ https://git-wip-us.apache.org/repos/asf?p=qpid-proton.git;h=c8c2775 ] PROTON-980: Enable handler processing the event after child handlers have processed it > Enable handler processing the event after child handlers have processed it > -- > > Key: PROTON-980 > URL: https://issues.apache.org/jira/browse/PROTON-980 > Project: Qpid Proton > Issue Type: Improvement > Components: proton-j >Affects Versions: 0.10 >Reporter: Bozo Dragojevic >Assignee: Bozo Dragojevic > Fix For: 0.11 > > > A Handler may wish to do further processing after all child handlers have > handled the event. Allow the handler to delegate the event to it's children. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
