This is the mail archive of the libc-hacker@sourceware.org mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix races in _nl_find_msg


Hi!

As the attached testcase shows (at least on quad core, perhaps not every
time but very often), BZ#5058 fixes didn't cure all races.  One problem
is that nconversions isn't reread after acquiring wrlock, which means that
several threads can use a stale cached nconversions value, which means they
won't be able to find the conversion added by another thread, and can
happily overwrite that entry.  The other problem is that when
convd->conv_tab is NULL, the setting of it to calloc returned address
isn't atomic nor guarded by any lock, so multiple threads can do that.
This can mean both memory leak, but also crash when the value is midway
through _nl_find_msg one value and then suddenly changes to a different one.

2008-03-30  Jakub Jelinek  <jakub@redhat.com>

	* intl/dcigettext.c (_nl_find_msg): Reread nconversions after
	acquiring wrlock.  Do conv_tab allocation while holding lock.
	* intl/Makefile: Add rules to build and run tst-gettext6.
	* intl/tst-gettext6.c: New test.
	* intl/tst-gettext6.sh: New file.

	* Makefile (LOCALES): Add ja_JP.UTF-8.

--- libc/intl/dcigettext.c	17 Nov 2007 07:37:30 -0000	1.58
+++ libc/intl/dcigettext.c	30 Mar 2008 21:42:59 -0000
@@ -879,6 +879,7 @@ _nl_find_msg (domain_file, domainbinding
 	{
 	  /* We have to allocate a new conversions table.  */
 	  __libc_rwlock_wrlock (domain->conversions_lock);
+	  nconversions = domain->nconversions;
 
 	  /* Maybe in the meantime somebody added the translation.
 	     Recheck.  */
@@ -1033,6 +1034,7 @@ _nl_find_msg (domain_file, domainbinding
 # endif
 	  )
 	{
+	  __libc_lock_define_initialized (static, lock)
 	  /* We are supposed to do a conversion.  First allocate an
 	     appropriate table with the same structure as the table
 	     of translations in the file, where we can put the pointers
@@ -1042,13 +1044,21 @@ _nl_find_msg (domain_file, domainbinding
 	     handle this case by converting RESULTLEN bytes, including
 	     NULs.  */
 
-	  if (convd->conv_tab == NULL
-	      && ((convd->conv_tab =
-		    (char **) calloc (nstrings + domain->n_sysdep_strings,
-				      sizeof (char *)))
-		  == NULL))
-	    /* Mark that we didn't succeed allocating a table.  */
-	    convd->conv_tab = (char **) -1;
+	  if (__builtin_expect (convd->conv_tab == NULL, 0))
+	    {
+	      __libc_lock_lock (lock);
+	      if (convd->conv_tab == NULL)
+		{
+		  convd->conv_tab
+		    = calloc (nstrings + domain->n_sysdep_strings,
+			      sizeof (char *));
+		  if (convd->conv_tab != NULL)
+		    goto not_translated_yet;
+		  /* Mark that we didn't succeed allocating a table.  */
+		  convd->conv_tab = (char **) -1;
+		}
+	      __libc_lock_unlock (lock);
+	    }
 
 	  if (__builtin_expect (convd->conv_tab == (char **) -1, 0))
 	    /* Nothing we can do, no more memory.  We cannot use the
@@ -1057,12 +1067,14 @@ _nl_find_msg (domain_file, domainbinding
 
 	  if (convd->conv_tab[act] == NULL)
 	    {
+	      __libc_lock_lock (lock);
+	    not_translated_yet:;
+
 	      /* We haven't used this string so far, so it is not
 		 translated yet.  Do this now.  */
 	      /* We use a bit more efficient memory handling.
 		 We allocate always larger blocks which get used over
 		 time.  This is faster than many small allocations.   */
-	      __libc_lock_define_initialized (static, lock)
 # define INITIAL_BLOCK_SIZE	4080
 	      static unsigned char *freemem;
 	      static size_t freemem_size;
@@ -1074,8 +1086,6 @@ _nl_find_msg (domain_file, domainbinding
 	      transmem_block_t *transmem_list = NULL;
 # endif
 
-	      __libc_lock_lock (lock);
-
 	      inbuf = (const unsigned char *) result;
 	      outbuf = freemem + sizeof (size_t);
 
--- libc/intl/Makefile	4 May 2005 17:53:42 -0000	1.54
+++ libc/intl/Makefile	30 Mar 2008 21:42:58 -0000
@@ -1,4 +1,4 @@
-# Copyright (C) 1995-2003, 2005 Free Software Foundation, Inc.
+# Copyright (C) 1995-2003, 2005, 2008 Free Software Foundation, Inc.
 # This file is part of the GNU C Library.
 
 # The GNU C Library is free software; you can redistribute it and/or
@@ -32,11 +32,11 @@ distribute = gmo.h gettextP.h hash-strin
 	     tst-codeset.sh tstcodeset.po \
 	     tst-gettext3.sh \
 	     tst-gettext4.sh tst-gettext4-de.po tst-gettext4-fr.po \
-	     tst-gettext5.sh
+	     tst-gettext5.sh tst-gettext6.sh
 
 include ../Makeconfig
 
-multithread-test-srcs := tst-gettext4 tst-gettext5
+multithread-test-srcs := tst-gettext4 tst-gettext5 tst-gettext6
 test-srcs := tst-gettext tst-translit tst-gettext2 tst-codeset tst-gettext3
 ifeq ($(have-thread-library),yes)
 test-srcs += $(multithread-test-srcs)
@@ -67,7 +67,8 @@ ifneq ($(strip $(MSGFMT)),:)
 tests: $(objpfx)tst-translit.out $(objpfx)tst-gettext2.out \
        $(objpfx)tst-codeset.out $(objpfx)tst-gettext3.out
 ifeq ($(have-thread-library),yes)
-tests: $(objpfx)tst-gettext4.out $(objpfx)tst-gettext5.out
+tests: $(objpfx)tst-gettext4.out $(objpfx)tst-gettext5.out \
+       $(objpfx)tst-gettext6.out
 endif
 ifneq (no,$(PERL))
 tests: $(objpfx)mtrace-tst-gettext
@@ -90,6 +91,8 @@ $(objpfx)tst-gettext4.out: tst-gettext4.
 	$(SHELL) -e $< $(common-objpfx) '$(run-program-prefix)' $(common-objpfx)intl/
 $(objpfx)tst-gettext5.out: tst-gettext5.sh $(objpfx)tst-gettext5
 	$(SHELL) -e $< $(common-objpfx) '$(run-program-prefix)' $(common-objpfx)intl/
+$(objpfx)tst-gettext6.out: tst-gettext6.sh $(objpfx)tst-gettext6
+	$(SHELL) -e $< $(common-objpfx) '$(run-program-prefix)' $(common-objpfx)intl/
 endif
 endif
 
@@ -104,6 +107,7 @@ CFLAGS-tst-codeset.c = -DOBJPFX=\"$(objp
 CFLAGS-tst-gettext3.c = -DOBJPFX=\"$(objpfx)\"
 CFLAGS-tst-gettext4.c = -DOBJPFX=\"$(objpfx)\"
 CFLAGS-tst-gettext5.c = -DOBJPFX=\"$(objpfx)\"
+CFLAGS-tst-gettext6.c = -DOBJPFX=\"$(objpfx)\"
 
 ifeq ($(have-thread-library),yes)
 ifeq (yes,$(build-shared))
@@ -122,6 +126,7 @@ $(objpfx)tst-codeset.out: $(objpfx)tst-g
 $(objpfx)tst-gettext3.out: $(objpfx)tst-gettext.out
 $(objpfx)tst-gettext4.out: $(objpfx)tst-gettext.out
 $(objpfx)tst-gettext5.out: $(objpfx)tst-gettext.out
+$(objpfx)tst-gettext6.out: $(objpfx)tst-gettext.out
 
 CPPFLAGS += -D'LOCALEDIR="$(msgcatdir)"' \
 	    -D'LOCALE_ALIAS_PATH="$(msgcatdir)"'
--- libc/intl/tst-gettext6.c	1 Jan 1970 00:00:00 -0000
+++ libc/intl/tst-gettext6.c	30 Mar 2008 21:42:59 -0000
@@ -0,0 +1,86 @@
+/* Test that gettext() in multithreaded applications works correctly.
+   Copyright (C) 2008 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Jakub Jelinek <jakub@redhat.com>, 2008.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <libintl.h>
+#include <locale.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+pthread_barrier_t b;
+
+static void *
+tf (void *arg)
+{
+  pthread_barrier_wait (&b);
+  return gettext ("Operation not permitted");
+}
+
+int
+test (void)
+{
+  pthread_t th[4];
+  unsetenv ("LANGUAGE");
+  unsetenv ("OUTPUT_CHARSET");
+  textdomain ("tstgettext6");
+  bindtextdomain ("tstgettext6", OBJPFX "domaindir");
+  setlocale (LC_ALL, "ja_JP.UTF-8");
+  pthread_barrier_init (&b, NULL, 4);
+  for (int i = 0; i < 4; i++)
+    if (pthread_create (&th[i], NULL, tf, NULL))
+      {
+	puts ("pthread_create failed");
+	return 1;
+      }
+  for (int i = 0; i < 4; i++)
+    pthread_join (th[i], NULL);
+  return 0;
+}
+
+int
+main (void)
+{
+  for (int i = 0; i < 300; i++)
+    {
+      pid_t p = fork ();
+      if (p == -1)
+	{
+	  printf ("fork failed: %m\n");
+	  return 1;
+	}
+      if (p == 0)
+	_exit (test ());
+      int status;
+      wait (&status);
+      if (WIFEXITED (status) && WEXITSTATUS (status) != 0)
+	{
+	  printf ("child exited with %d\n", WEXITSTATUS (status));
+	  return 1;
+	}
+      else if (WIFSIGNALED (status))
+	{
+	  printf ("child killed by signal %d\n", WTERMSIG (status));
+	  return 1;
+	}
+    }
+  return 0;
+}
--- libc/intl/tst-gettext6.sh	1 Jan 1970 00:00:00 -0000
+++ libc/intl/tst-gettext6.sh	30 Mar 2008 21:42:59 -0000
@@ -0,0 +1,41 @@
+#! /bin/sh
+# Test that gettext() in multithreaded applications works correctly.
+# Copyright (C) 2008 Free Software Foundation, Inc.
+# This file is part of the GNU C Library.
+#
+
+# The GNU C Library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+
+# The GNU C Library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+
+# You should have received a copy of the GNU Lesser General Public
+# License along with the GNU C Library; if not, write to the Free
+# Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+# 02111-1307 USA.
+
+common_objpfx=$1
+run_program_prefix=$2
+objpfx=$3
+
+LC_ALL=C
+export LC_ALL
+
+# Create the domain directory.
+mkdir -p ${objpfx}domaindir/ja_JP/LC_MESSAGES
+# Populate it.
+msgfmt -o ${objpfx}domaindir/ja_JP/LC_MESSAGES/tstgettext6.mo ../po/ja.po
+
+GCONV_PATH=${common_objpfx}iconvdata
+export GCONV_PATH
+LOCPATH=${common_objpfx}localedata
+export LOCPATH
+
+${run_program_prefix} ${objpfx}tst-gettext6 > ${objpfx}tst-gettext6.out
+
+exit $?
--- libc/localedata/Makefile	2 Oct 2007 17:19:40 -0000	1.107
+++ libc/localedata/Makefile	30 Mar 2008 21:46:25 -0000
@@ -133,7 +133,8 @@ LOCALES := de_DE.ISO-8859-1 de_DE.UTF-8 
 	   en_US.ISO-8859-1 ja_JP.EUC-JP da_DK.ISO-8859-1 \
 	   hr_HR.ISO-8859-2 sv_SE.ISO-8859-1 ja_JP.SJIS fr_FR.ISO-8859-1 \
 	   vi_VN.TCVN5712-1 nb_NO.ISO-8859-1 nn_NO.ISO-8859-1 \
-	   tr_TR.UTF-8 cs_CZ.UTF-8 zh_TW.EUC-TW fa_IR.UTF-8 fr_FR.UTF-8
+	   tr_TR.UTF-8 cs_CZ.UTF-8 zh_TW.EUC-TW fa_IR.UTF-8 fr_FR.UTF-8 \
+	   ja_JP.UTF-8
 LOCALE_SRCS := $(shell echo "$(LOCALES)"|sed 's/\([^ .]*\)[^ ]*/\1/g')
 CHARMAPS := $(shell echo "$(LOCALES)" | \
 		    sed -e 's/[^ .]*[.]\([^ ]*\)/\1/g' -e s/SJIS/SHIFT_JIS/g)

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]