Re: regexp_exec thread-unsafe

[Paul Eggert]

On 05/14/2013 02:21 PM, Ludovic Courtès wrote:

> How should that be fixed?  Shouldn’t __libc_lock_unlock & co. be rebased
> on top of pthread_mutex_t?

Yes, thanks, that sounds right.  I pushed the following patch into gnulib;
could you please see whether it fixes the problem for you?  It'd be
helpful to try it on non-glibc systems too, if possible.

---
 ChangeLog            | 19 +++++++++++++++++++
 lib/regcomp.c        | 11 ++++++++---
 lib/regex_internal.h | 23 +++++++++++++++++------
 lib/regexec.c        |  4 ----
 modules/regex        |  1 +
 5 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 08042c5..35b6dd2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,24 @@
 2013-05-19  Paul Eggert  <address@hidden>
 
+       regex: fix dfa race in multithreaded uses
+       Problem reported by Ludovic Courtès in
+       <http://lists.gnu.org/archive/html/bug-gnulib/2013-05/msg00058.html>.
+       * lib/regex_internal.h (lock_define, lock_init, lock_fini):
+       New macros.  All uses of __libc_lock_define, __libc_lock_init
+       changed to use the first two of these.
+       (__libc_lock_lock, __libc_lock_unlock): New macros, for
+       non-glibc platforms.
+       (struct re_dfa_t): Define the lock unconditionally.
+       * lib/regexec.c (regexec, re_search_stub): Remove some now-incorrect
+       '#ifdef _LIBC"s.
+       * modules/regex (Depends-on): Add pthread, if we use the
+       included regex.
+
+       * lib/regcomp.c: Do actions that are not needed for glibc,
+       but may be needed elsewhere.
+       (regfree, re_compile_internal): Destroy the lock.
+       (re_compile_internal): Check for lock-initialization failure.
+
        malloca: port to compilers that reject size-zero arrays
        This fixes a bug introduced in my previous patch.
        * lib/malloca.c (struct preliminary_header): Use an int
diff --git a/lib/regcomp.c b/lib/regcomp.c
index 4a8485e..5344381 100644
--- a/lib/regcomp.c
+++ b/lib/regcomp.c
@@ -663,7 +663,10 @@ regfree (preg)
 {
   re_dfa_t *dfa = preg->buffer;
   if (BE (dfa != NULL, 1))
-    free_dfa_content (dfa);
+    {
+      lock_fini (dfa->lock);
+      free_dfa_content (dfa);
+    }
   preg->buffer = NULL;
   preg->allocated = 0;
 
@@ -784,6 +787,8 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
   preg->used = sizeof (re_dfa_t);
 
   err = init_dfa (dfa, length);
+  if (BE (err == REG_NOERROR && lock_init (dfa->lock) != 0, 0))
+    err = REG_ESPACE;
   if (BE (err != REG_NOERROR, 0))
     {
       free_dfa_content (dfa);
@@ -797,8 +802,6 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
   strncpy (dfa->re_str, pattern, length + 1);
 #endif
 
-  __libc_lock_init (dfa->lock);
-
   err = re_string_construct (&regexp, pattern, length, preg->translate,
                             (syntax & RE_ICASE) != 0, dfa);
   if (BE (err != REG_NOERROR, 0))
@@ -806,6 +809,7 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
     re_compile_internal_free_return:
       free_workarea_compile (preg);
       re_string_destruct (&regexp);
+      lock_fini (dfa->lock);
       free_dfa_content (dfa);
       preg->buffer = NULL;
       preg->allocated = 0;
@@ -838,6 +842,7 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
 
   if (BE (err != REG_NOERROR, 0))
     {
+      lock_fini (dfa->lock);
       free_dfa_content (dfa);
       preg->buffer = NULL;
       preg->allocated = 0;
diff --git a/lib/regex_internal.h b/lib/regex_internal.h
index 439444c..63a9979 100644
--- a/lib/regex_internal.h
+++ b/lib/regex_internal.h
@@ -32,12 +32,25 @@
 #include <wctype.h>
 #include <stdbool.h>
 #include <stdint.h>
+
 #if defined _LIBC
 # include <bits/libc-lock.h>
+#endif
+/* Use __libc_lock_define (, NAME) if the library defines the macro,
+   and if the compiler is known to support empty macro arguments.  */
+#if (defined __libc_lock_define                                         \
+     && ((defined __GNUC__ && !defined __STRICT_ANSI__)                 \
+         || (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__)))
+# define lock_define(name) __libc_lock_define (, name)
+# define lock_init(lock) (__libc_lock_init (lock), 0)
+# define lock_fini(lock) 0
 #else
-# define __libc_lock_init(NAME) do { } while (0)
-# define __libc_lock_lock(NAME) do { } while (0)
-# define __libc_lock_unlock(NAME) do { } while (0)
+# include <pthread.h>
+# define lock_define(name) pthread_mutex_t name;
+# define lock_init(lock) pthread_mutex_init (&(lock), 0)
+# define lock_fini(lock) pthread_mutex_destroy (&(lock))
+# define __libc_lock_lock(lock) pthread_mutex_lock (&(lock))
+# define __libc_lock_unlock(lock) pthread_mutex_unlock (&(lock))
 #endif
 
 /* In case that the system doesn't have isblank().  */
@@ -698,9 +711,7 @@ struct re_dfa_t
 #ifdef DEBUG
   char* re_str;
 #endif
-#ifdef _LIBC
-  __libc_lock_define (, lock)
-#endif
+  lock_define (lock)
 };
 
 #define re_node_set_init_empty(set) memset (set, '\0', sizeof (re_node_set))
diff --git a/lib/regexec.c b/lib/regexec.c
index 09c3eec..114287e 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -228,9 +228,7 @@ regexec (preg, string, nmatch, pmatch, eflags)
 {
   reg_errcode_t err;
   Idx start, length;
-#ifdef _LIBC
   re_dfa_t *dfa = preg->buffer;
-#endif
 
   if (eflags & ~(REG_NOTBOL | REG_NOTEOL | REG_STARTEND))
     return REG_BADPAT;
@@ -421,9 +419,7 @@ re_search_stub (struct re_pattern_buffer *bufp,
   Idx nregs;
   regoff_t rval;
   int eflags = 0;
-#ifdef _LIBC
   re_dfa_t *dfa = bufp->buffer;
-#endif
   Idx last_start = start + range;
 
   /* Check for out-of-range.  */
diff --git a/modules/regex b/modules/regex
index 8f5eda0..2dbb777 100644
--- a/modules/regex
+++ b/modules/regex
@@ -24,6 +24,7 @@ memmove         [test $ac_use_included_regex = yes]
 mbrtowc         [test $ac_use_included_regex = yes]
 mbsinit         [test $ac_use_included_regex = yes]
 nl_langinfo     [test $ac_use_included_regex = yes]
+pthread         [test $ac_use_included_regex = yes]
 stdbool         [test $ac_use_included_regex = yes]
 stdint          [test $ac_use_included_regex = yes]
 wchar           [test $ac_use_included_regex = yes]
-- 
1.7.11.7