From c9c15ac3162d10a999bf71418fd710bf5676175e Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Tue, 21 May 2019 10:19:46 +0200
Subject: [PATCH] wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584]

This also adds an overflow check and documents the synchronization
requirement in <gconv.h>.
---
 ChangeLog           |  9 +++++++++
 iconv/gconv.h       |  2 ++
 wcsmbs/wcsmbsload.c | 22 ++++++++++++++++++----
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1dcb620..35e6f7c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2019-05-21  Florian Weimer  <fweimer@redhat.com>
 
+	[BZ #24584]
+	* wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Acquire __gconv_lock
+	before updating __counter field and release it afterwards.  Add
+	overflow check.
+	* iconv/gconv.h (struct __gconv_step): Mention synchronization
+	requirement for __counter member.
+
+2019-05-21  Florian Weimer  <fweimer@redhat.com>
+
 	[BZ #24583]
 	* wcsmbs/wcsmbsload.h (__wcsmbs_close_conv): Declare.
 	* wcsmbs/wcsmbsload.c (__wcsmbs_close_conv): Define.
diff --git a/iconv/gconv.h b/iconv/gconv.h
index 5ad26c0..7ce79bc 100644
--- a/iconv/gconv.h
+++ b/iconv/gconv.h
@@ -86,6 +86,8 @@ struct __gconv_step
   struct __gconv_loaded_object *__shlib_handle;
   const char *__modname;
 
+  /* For internal use by glibc.  (Accesses to this member must occur
+     when the internal __gconv_lock mutex is acquired).  */
   int __counter;
 
   char *__from_name;
diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
index 10e1a4f..840d4ab 100644
--- a/wcsmbs/wcsmbsload.c
+++ b/wcsmbs/wcsmbsload.c
@@ -20,6 +20,7 @@
 #include <langinfo.h>
 #include <limits.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <string.h>
 
 #include <locale/localeinfo.h>
@@ -223,12 +224,25 @@ __wcsmbs_clone_conv (struct gconv_fcts *copy)
   /* Copy the data.  */
   *copy = *orig;
 
-  /* Now increment the usage counters.
-     Note: This assumes copy->*_nsteps == 1.  */
+  /* Now increment the usage counters.  Note: This assumes
+     copy->*_nsteps == 1.  The current locale holds a reference, so it
+     is still there after acquiring the lock.  */
+
+  __libc_lock_lock (__gconv_lock);
+
+  bool overflow = false;
   if (copy->towc->__shlib_handle != NULL)
-    ++copy->towc->__counter;
+    overflow |= __builtin_add_overflow (copy->towc->__counter, 1,
+					&copy->towc->__counter);
   if (copy->tomb->__shlib_handle != NULL)
-    ++copy->tomb->__counter;
+    overflow |= __builtin_add_overflow (copy->tomb->__counter, 1,
+					&copy->tomb->__counter);
+
+  __libc_lock_unlock (__gconv_lock);
+
+  if (overflow)
+    __libc_fatal ("\
+Fatal glibc error: gconv module reference counter overflow\n");
 }
 
 
-- 
2.7.4