[alsa-devel] [PATCH 0/2] Fix thread safety issues
This is a set of two patches to fix thread safety issues I found when running a video application over ALSA. The application uses LibVLC to play several videos simultaneously. LibVLC calls snd_pcm_open() and snd_device_name_hint() from multiple threads, without locking--which is allowed according to http://www.alsa-project.org/main/index.php/SMP_Design.
Without these patches, I would get random error messages like:
ALSA lib confmisc.c:768:(parse_card) cannot find card '$CARD' ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_card_driver returned error: No such device ALSA lib confmisc.c:392:(snd_func_concat) error evaluating strings ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_concat returned error: No such device ALSA lib confmisc.c:1251:(snd_func_refer) error evaluating name ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_refer returned error: No such device ALSA lib conf.c:4663:(snd_config_expand) Evaluate error: No such device
Jerome Forissier (2): Make snd_device_name_hint() thread-safe by locking a mutex snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r()
src/control/namehint.c | 31 +++++++++++++++++++++++++++++++ src/pcm/pcm_direct.c | 15 +++++++++++---- 2 files changed, 42 insertions(+), 4 deletions(-)
Signed-off-by: Jerome Forissier jerome@taodyne.com --- src/control/namehint.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/src/control/namehint.c b/src/control/namehint.c index 19352be..b30f75d 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -26,6 +26,9 @@ */
#include "local.h" +#ifdef HAVE_LIBPTHREAD +#include <pthread.h> +#endif
#ifndef DOC_HIDDEN struct hint_list { @@ -44,6 +47,25 @@ struct hint_list { int show_all; char *cardname; }; + +#ifdef HAVE_LIBPTHREAD +static pthread_mutex_t snd_device_name_hint_mutex = + PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; +#endif + +static inline void snd_device_name_hint_lock() +{ +#ifdef HAVE_LIBPTHREAD + pthread_mutex_lock(&snd_device_name_hint_mutex); +#endif +} + +static inline void snd_device_name_hint_unlock() +{ +#ifdef HAVE_LIBPTHREAD + pthread_mutex_unlock(&snd_device_name_hint_mutex); +#endif +} #endif
static int hint_list_add(struct hint_list *list, @@ -553,9 +575,13 @@ int snd_device_name_hint(int card, const char *iface, void ***hints)
if (hints == NULL) return -EINVAL; + snd_device_name_hint_lock(); err = snd_config_update(); if (err < 0) + { + snd_device_name_hint_unlock(); return err; + } list.list = NULL; list.count = list.allocated = 0; list.siface = iface; @@ -574,7 +600,10 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) else if (strcmp(iface, "ctl") == 0) list.iface = SND_CTL_ELEM_IFACE_MIXER; else + { + snd_device_name_hint_unlock(); return -EINVAL; + } list.show_all = 0; list.cardname = NULL; if (snd_config_search(snd_config, "defaults.namehint.showall", &conf) >= 0) @@ -618,6 +647,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) snd_device_name_free_hint((void **)list.list); if (list.cardname) free(list.cardname); + snd_device_name_hint_unlock(); return err; } else { err = hint_list_add(&list, NULL, NULL); @@ -627,6 +657,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (list.cardname) free(list.cardname); } + snd_device_name_hint_unlock(); return 0; }
Fixes a thread safety issue with snd_pcm_open().
Signed-off-by: Jerome Forissier jerome@taodyne.com --- src/pcm/pcm_direct.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 6802ecc..38c6c66 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -1629,13 +1629,20 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, continue; } if (isdigit(*group) == 0) { - struct group *grp = getgrnam(group); - if (grp == NULL) { + long clen = sysconf(_SC_GETGR_R_SIZE_MAX); + size_t len = (clen == -1) ? 1024 : (size_t)clen; + struct group grp, *pgrp; + char *buffer = (char *)malloc(len); + if (buffer == NULL) + return -ENOMEM; + int st = getgrnam_r(group, &grp, buffer, len, &pgrp); + if (st != 0) { SNDERR("The field ipc_gid must be a valid group (create group %s)", group); - free(group); + free(buffer); return -EINVAL; } - rec->ipc_gid = grp->gr_gid; + rec->ipc_gid = pgrp->gr_gid; + free(buffer); } else { rec->ipc_gid = strtol(group, &endp, 10); }
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
This is a set of two patches to fix thread safety issues I found when running a video application over ALSA. The application uses LibVLC to play several videos simultaneously. LibVLC calls snd_pcm_open() and snd_device_name_hint() from multiple threads, without locking--which is allowed according to http://www.alsa-project.org/main/index.php/SMP_Design.
The fix for snd_pcm_direct_parse_open_conf() looks good. I'm going to apply this now.
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
thanks,
Takashi
Without these patches, I would get random error messages like:
ALSA lib confmisc.c:768:(parse_card) cannot find card '$CARD' ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_card_driver returned error: No such device ALSA lib confmisc.c:392:(snd_func_concat) error evaluating strings ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_concat returned error: No such device ALSA lib confmisc.c:1251:(snd_func_refer) error evaluating name ALSA lib conf.c:4184:(_snd_config_evaluate) function snd_func_refer returned error: No such device ALSA lib conf.c:4663:(snd_config_expand) Evaluate error: No such device
Jerome Forissier (2): Make snd_device_name_hint() thread-safe by locking a mutex snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r()
src/control/namehint.c | 31 +++++++++++++++++++++++++++++++ src/pcm/pcm_direct.c | 15 +++++++++++---- 2 files changed, 42 insertions(+), 4 deletions(-)
-- 1.8.1.2
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 30 janv. 2013, at 17:03, Takashi Iwai wrote:
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
This is a set of two patches to fix thread safety issues I found when running a video application over ALSA. The application uses LibVLC to play several videos simultaneously. LibVLC calls snd_pcm_open() and snd_device_name_hint() from multiple threads, without locking--which is allowed according to http://www.alsa-project.org/main/index.php/SMP_Design.
The fix for snd_pcm_direct_parse_open_conf() looks good. I'm going to apply this now.
Good, thank you :)
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
I tried following this path, but there is a problem: try_config() calls snd_lib_error_set_handler() so I fail to see how we can make this code reentrant. In fact, the random errors I've seen when running my multi-threaded test were probably just a side-effect of fiddling with the global error handler from multiple threads. Probably harmless, but still it makes snd_device_name_hint() not thread-safe, strictly speaking.
Any suggestion? Should I consider moving snd_lib_error to thread-local storage?
Thanks.
At Wed, 30 Jan 2013 19:13:54 +0100, Jérôme Forissier wrote:
On 30 janv. 2013, at 17:03, Takashi Iwai wrote:
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
This is a set of two patches to fix thread safety issues I found when running a video application over ALSA. The application uses LibVLC to play several videos simultaneously. LibVLC calls snd_pcm_open() and snd_device_name_hint() from multiple threads, without locking--which is allowed according to http://www.alsa-project.org/main/index.php/SMP_Design.
The fix for snd_pcm_direct_parse_open_conf() looks good. I'm going to apply this now.
Good, thank you :)
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
I tried following this path, but there is a problem: try_config() calls snd_lib_error_set_handler() so I fail to see how we can make this code reentrant. In fact, the random errors I've seen when running my multi-threaded test were probably just a side-effect of fiddling with the global error handler from multiple threads. Probably harmless, but still it makes snd_device_name_hint() not thread-safe, strictly speaking.
Yeah, I see the issue now, too.
The problem is that the error handler is really global, not only used in conf.c and co. Thus applying a lock locally doesn't suffice.
Any suggestion? Should I consider moving snd_lib_error to thread-local storage?
TLB would be an option, indeed. But blindly moving snd_lib_error to TLS breaks the current behavior, so we should avoid it.
How about a patch like below? It adds another error redirection. Unless local_error is set, the behavior is identical.
(Maybe we can name it better, yeah. I just wanted to avoid to put a word "thread" in the function because it should be usable in both thread- and non-thread-capable environment.)
thanks,
Takashi
--- diff --git a/include/error.h b/include/error.h index 6d27083..256cd5f 100644 --- a/include/error.h +++ b/include/error.h @@ -74,5 +74,11 @@ extern int snd_lib_error_set_handler(snd_lib_error_handler_t handler); } #endif
+typedef void (*snd_local_error_handler_t)(const char *file, int line, + const char *func, int err, + const char *fmt, va_list arg); + +snd_local_error_handler_t snd_lib_error_set_local(snd_local_error_handler_t func); + #endif /* __ALSA_ERROR_H */
diff --git a/src/control/namehint.c b/src/control/namehint.c index 19352be..defc036 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -80,7 +80,8 @@ static void zero_handler(const char *file ATTRIBUTE_UNUSED, int line ATTRIBUTE_UNUSED, const char *function ATTRIBUTE_UNUSED, int err ATTRIBUTE_UNUSED, - const char *fmt ATTRIBUTE_UNUSED, ...) + const char *fmt ATTRIBUTE_UNUSED, + va_list arg ATTRIBUTE_UNUSED) { }
@@ -212,7 +213,7 @@ static int try_config(struct hint_list *list, const char *base, const char *name) { - snd_lib_error_handler_t eh; + snd_local_error_handler_t eh; snd_config_t *res = NULL, *cfg, *cfg1, *n; snd_config_iterator_t i, next; char *buf, *buf1 = NULL, *buf2; @@ -239,10 +240,9 @@ static int try_config(struct hint_list *list, sprintf(buf, "%s:CARD=%s", name, snd_ctl_card_info_get_id(list->info)); else strcpy(buf, name); - eh = snd_lib_error; - snd_lib_error_set_handler(&zero_handler); + eh = snd_lib_error_set_local(&zero_handler); err = snd_config_search_definition(snd_config, base, buf, &res); - snd_lib_error_set_handler(eh); + snd_lib_error_set_local(eh); if (err < 0) goto __skip_add; cleanup_res = 1; @@ -337,10 +337,9 @@ static int try_config(struct hint_list *list, goto __ok; /* find, if all parameters have a default, */ /* otherwise filter this definition */ - eh = snd_lib_error; - snd_lib_error_set_handler(&zero_handler); + eh = snd_lib_error_set_local(&zero_handler); err = snd_config_search_alias_hooks(snd_config, base, buf, &res); - snd_lib_error_set_handler(eh); + snd_lib_error_set_local(eh); if (err < 0) goto __cleanup; if (snd_config_search(res, "@args", &cfg) >= 0) { diff --git a/src/error.c b/src/error.c index 7d5f509..6026ede 100644 --- a/src/error.c +++ b/src/error.c @@ -60,6 +60,23 @@ const char *snd_strerror(int errnum) return snd_error_codes[errnum]; }
+#define SUPPORT_TLS /* XXX should be checked in configure */ + +#ifdef SUPPORT_TLS +#define TLS_PFX __thread +#else +#define TLS_PFX /* NOP */ +#endif + +static TLS_PFX snd_local_error_handler_t local_error; + +snd_local_error_handler_t snd_lib_error_set_local(snd_local_error_handler_t func) +{ + snd_local_error_handler_t old = local_error; + local_error = func; + return old; +} + /** * \brief The default error handler function. * \param file The filename where the error was hit. @@ -75,6 +92,11 @@ static void snd_lib_error_default(const char *file, int line, const char *functi { va_list arg; va_start(arg, fmt); + if (local_error) { + local_error(file, line, function, err, fmt, arg); + va_end(arg); + return; + } fprintf(stderr, "ALSA lib %s:%i:(%s) ", file, line, function); vfprintf(stderr, fmt, arg); if (err)
On 31 janv. 2013, at 09:54, Takashi Iwai wrote:
At Wed, 30 Jan 2013 19:13:54 +0100, Jérôme Forissier wrote:
On 30 janv. 2013, at 17:03, Takashi Iwai wrote:
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
<...>
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
I tried following this path, but there is a problem: try_config() calls snd_lib_error_set_handler() so I fail to see how we can make this code reentrant. In fact, the random errors I've seen when running my multi-threaded test were probably just a side-effect of fiddling with the global error handler from multiple threads. Probably harmless, but still it makes snd_device_name_hint() not thread-safe, strictly speaking.
Yeah, I see the issue now, too.
The problem is that the error handler is really global, not only used in conf.c and co. Thus applying a lock locally doesn't suffice.
Any suggestion? Should I consider moving snd_lib_error to thread-local storage?
TLB would be an option, indeed. But blindly moving snd_lib_error to TLS breaks the current behavior, so we should avoid it.
One could argue that the current behavior with multi-threaded apps is undefined anyways. But I see what you mean.
How about a patch like below? It adds another error redirection. Unless local_error is set, the behavior is identical.
Nice suggestion. Will try that. After all, "all problems in computer science can be solved by another level of indirection..." ;-)
(Maybe we can name it better, yeah. I just wanted to avoid to put a word "thread" in the function because it should be usable in both thread- and non-thread-capable environment.)
Well you can consider that, in a non-threaded environment, there is just one thread (the main thread).
Thanks for your help. I will post an updated patch when done.
At Thu, 31 Jan 2013 10:48:27 +0100, Jérôme Forissier wrote:
On 31 janv. 2013, at 09:54, Takashi Iwai wrote:
At Wed, 30 Jan 2013 19:13:54 +0100, Jérôme Forissier wrote:
On 30 janv. 2013, at 17:03, Takashi Iwai wrote:
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
<...>
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
I tried following this path, but there is a problem: try_config() calls snd_lib_error_set_handler() so I fail to see how we can make this code reentrant. In fact, the random errors I've seen when running my multi-threaded test were probably just a side-effect of fiddling with the global error handler from multiple threads. Probably harmless, but still it makes snd_device_name_hint() not thread-safe, strictly speaking.
Yeah, I see the issue now, too.
The problem is that the error handler is really global, not only used in conf.c and co. Thus applying a lock locally doesn't suffice.
Any suggestion? Should I consider moving snd_lib_error to thread-local storage?
TLB would be an option, indeed. But blindly moving snd_lib_error to TLS breaks the current behavior, so we should avoid it.
One could argue that the current behavior with multi-threaded apps is undefined anyways.
Yes, it was simply badly designed like errno. We can't change it at this point, but put a clear statement at least.
If so, the web page should updated, too. Jaroslav, could you add some texts like below? (Feel free to rephrase.)
"There are a few global variables that are used in alsa-lib, such as the global error handler pointer and the global root config pointer. Accessing / modifying these would be thread-unsafe."
But I see what you mean.
How about a patch like below? It adds another error redirection. Unless local_error is set, the behavior is identical.
Nice suggestion. Will try that. After all, "all problems in computer science can be solved by another level of indirection..." ;-)
Hehe.
(Maybe we can name it better, yeah. I just wanted to avoid to put a word "thread" in the function because it should be usable in both thread- and non-thread-capable environment.)
Well you can consider that, in a non-threaded environment, there is just one thread (the main thread).
Hm, one can call it so, too. I don't mind much how it's named. Spell it if you have a better one.
thanks,
Takashi
Thanks for your help. I will post an updated patch when done.
-- Jerome
Date 31.1.2013 11:09, Takashi Iwai wrote:
Jaroslav, could you add some texts like below? (Feel free to rephrase.)
"There are a few global variables that are used in alsa-lib, such as the global error handler pointer and the global root config pointer. Accessing / modifying these would be thread-unsafe."
Added.
Jaroslav
participants (3)
-
Jaroslav Kysela -
Jerome Forissier -
Takashi Iwai