[PATCH 0/2] Fix thread safety issues

[PATCH 0/2] Fix thread safety issues

[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

[PATCH 1/2] Make snd_device_name_hint() thread-safe by locking a mutex

[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

[PATCH 2/2] snd_pcm_direct_parse_open_conf(): use thread-safe getgrnam_r()

[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

Re: [PATCH 0/2] Fix thread safety issues

[Takashi Iwai]

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
> 

Re: [PATCH 0/2] Fix thread safety issues

[Jérôme Forissier]

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

Re: [PATCH 0/2] Fix thread safety issues

[Takashi Iwai]

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

Re: [PATCH 0/2] Fix thread safety issues

[Jérôme Forissier]

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

Re: [PATCH 0/2] Fix thread safety issues

[Takashi Iwai]

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

Re: [PATCH 0/2] Fix thread safety issues

[Jaroslav Kysela]

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.