* [PATCH 0/2] Fix thread safety issues
@ 2013-01-30 15:22 Jerome Forissier
2013-01-30 15:22 ` [PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex Jerome Forissier
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jerome Forissier @ 2013-01-30 15:22 UTC (permalink / raw)
To: alsa-devel; +Cc: Jerome Forissier
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(-)
--
1.8.1.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex 2013-01-30 15:22 [PATCH 0/2] Fix thread safety issues Jerome Forissier @ 2013-01-30 15:22 ` Jerome Forissier 2013-01-30 15:22 ` [PATCH 2/2] snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r() Jerome Forissier 2013-01-30 16:03 ` [PATCH 0/2] Fix thread safety issues Takashi Iwai 2 siblings, 0 replies; 9+ messages in thread From: Jerome Forissier @ 2013-01-30 15:22 UTC (permalink / raw) To: alsa-devel; +Cc: Jerome Forissier 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; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r() 2013-01-30 15:22 [PATCH 0/2] Fix thread safety issues Jerome Forissier 2013-01-30 15:22 ` [PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex Jerome Forissier @ 2013-01-30 15:22 ` Jerome Forissier 2013-01-30 16:03 ` [PATCH 0/2] Fix thread safety issues Takashi Iwai 2 siblings, 0 replies; 9+ messages in thread From: Jerome Forissier @ 2013-01-30 15:22 UTC (permalink / raw) To: alsa-devel; +Cc: Jerome Forissier 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); } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-30 15:22 [PATCH 0/2] Fix thread safety issues Jerome Forissier 2013-01-30 15:22 ` [PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex Jerome Forissier 2013-01-30 15:22 ` [PATCH 2/2] snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r() Jerome Forissier @ 2013-01-30 16:03 ` Takashi Iwai 2013-01-30 18:13 ` Jérôme Forissier 2 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2013-01-30 16:03 UTC (permalink / raw) To: Jerome Forissier; +Cc: alsa-devel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-30 16:03 ` [PATCH 0/2] Fix thread safety issues Takashi Iwai @ 2013-01-30 18:13 ` Jérôme Forissier 2013-01-31 8:54 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: Jérôme Forissier @ 2013-01-30 18:13 UTC (permalink / raw) To: Takashi Iwai; +Cc: 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. -- Jerome ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-30 18:13 ` Jérôme Forissier @ 2013-01-31 8:54 ` Takashi Iwai 2013-01-31 9:48 ` Jérôme Forissier 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2013-01-31 8:54 UTC (permalink / raw) To: Jérôme Forissier; +Cc: alsa-devel 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) _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-31 8:54 ` Takashi Iwai @ 2013-01-31 9:48 ` Jérôme Forissier 2013-01-31 10:09 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: Jérôme Forissier @ 2013-01-31 9:48 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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. -- Jerome ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-31 9:48 ` Jérôme Forissier @ 2013-01-31 10:09 ` Takashi Iwai 2013-01-31 11:38 ` Jaroslav Kysela 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2013-01-31 10:09 UTC (permalink / raw) To: Jérôme Forissier; +Cc: alsa-devel 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 > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Fix thread safety issues 2013-01-31 10:09 ` Takashi Iwai @ 2013-01-31 11:38 ` Jaroslav Kysela 0 siblings, 0 replies; 9+ messages in thread From: Jaroslav Kysela @ 2013-01-31 11:38 UTC (permalink / raw) To: Takashi Iwai; +Cc: ALSA development 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 -- Jaroslav Kysela <perex@perex.cz> Linux Kernel Sound Maintainer ALSA Project; Red Hat, Inc. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-01-31 11:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-30 15:22 [PATCH 0/2] Fix thread safety issues Jerome Forissier 2013-01-30 15:22 ` [PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex Jerome Forissier 2013-01-30 15:22 ` [PATCH 2/2] snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r() Jerome Forissier 2013-01-30 16:03 ` [PATCH 0/2] Fix thread safety issues Takashi Iwai 2013-01-30 18:13 ` Jérôme Forissier 2013-01-31 8:54 ` Takashi Iwai 2013-01-31 9:48 ` Jérôme Forissier 2013-01-31 10:09 ` Takashi Iwai 2013-01-31 11:38 ` Jaroslav Kysela
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).