[alsa-devel] [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)