Remove unnecessary locking when reading iconv configuration [BZ #22062]
In iconv/gconv_conf.c, __gconv_get_path unnecessarily obtains a lock when populating the array pointed to by __gconv_path_elem. The locking is not necessary because all calls to __gconv_read_conf (which in turn calls __gconv_get_path) are serialized using __libc_once. This patch: - removes all locking in __gconv_get_path; - replaces all explicitly serialized __gconv_read_conf calls with calls to __gconv_load_conf, a new wrapper that is serialized internally; - adds a new test, iconv/tst-iconv_mt.c, to exercise iconv initialization, usage, and cleanup in a multi-threaded program; - indents __gconv_get_path correctly, removing tab characters (which makes the patch look a little bigger than it really is). After removing the unnecessary locking, it was confirmed that the test case fails if the relevant __libc_once is removed. Additionally, four localedata and iconvdata tests also fail. This gives confidence that the testsuite sufficiently guards against some regressions relating to multi-threading with iconv. Tested on x86_64 and i686.
This commit is contained in:
parent
729f34028a
commit
c5288d378a
16
ChangeLog
16
ChangeLog
|
@ -1,3 +1,19 @@
|
||||||
|
2018-10-17 Arjun Shankar <arjun@redhat.com>
|
||||||
|
|
||||||
|
[BZ #22062]
|
||||||
|
* iconv/gconv_conf.c (__gconv_get_path): Remove locking and fix
|
||||||
|
indentation.
|
||||||
|
* (__gconv_read_conf): Mark function static.
|
||||||
|
* (once): New static variable.
|
||||||
|
* (__gconv_load_conf): New function.
|
||||||
|
* iconv/gconv_int.h (__gconv_load_conf): Likewise.
|
||||||
|
* iconv/gconv_db.c (once): Remove static variable.
|
||||||
|
* (__gconv_compare_alias): Use __gconv_load_conf instead of
|
||||||
|
__gconv_read_conf.
|
||||||
|
* (__gconv_find_transform): Likewise.
|
||||||
|
* iconv/tst-iconv-mt.c: New test.
|
||||||
|
* iconv/Makefile: Add tst-iconv_mt.
|
||||||
|
|
||||||
2018-10-17 Joseph Myers <joseph@codesourcery.com>
|
2018-10-17 Joseph Myers <joseph@codesourcery.com>
|
||||||
|
|
||||||
* sysdeps/unix/sysv/linux/Makefile (sysdep_headers): Add
|
* sysdeps/unix/sysv/linux/Makefile (sysdep_headers): Add
|
||||||
|
|
|
@ -43,7 +43,8 @@ CFLAGS-charmap.c += -DCHARMAP_PATH='"$(i18ndir)/charmaps"' \
|
||||||
CFLAGS-linereader.c += -DNO_TRANSLITERATION
|
CFLAGS-linereader.c += -DNO_TRANSLITERATION
|
||||||
CFLAGS-simple-hash.c += -I../locale
|
CFLAGS-simple-hash.c += -I../locale
|
||||||
|
|
||||||
tests = tst-iconv1 tst-iconv2 tst-iconv3 tst-iconv4 tst-iconv5 tst-iconv6
|
tests = tst-iconv1 tst-iconv2 tst-iconv3 tst-iconv4 tst-iconv5 tst-iconv6 \
|
||||||
|
tst-iconv-mt
|
||||||
|
|
||||||
others = iconv_prog iconvconfig
|
others = iconv_prog iconvconfig
|
||||||
install-others-programs = $(inst_bindir)/iconv
|
install-others-programs = $(inst_bindir)/iconv
|
||||||
|
@ -67,6 +68,8 @@ endif
|
||||||
$(objpfx)gconv-modules: test-gconv-modules
|
$(objpfx)gconv-modules: test-gconv-modules
|
||||||
cp $< $@
|
cp $< $@
|
||||||
|
|
||||||
|
$(objpfx)tst-iconv-mt: $(shared-thread-library)
|
||||||
|
|
||||||
ifeq (yes,$(build-shared))
|
ifeq (yes,$(build-shared))
|
||||||
tests += tst-gconv-init-failure
|
tests += tst-gconv-init-failure
|
||||||
modules-names += tst-gconv-init-failure-mod
|
modules-names += tst-gconv-init-failure-mod
|
||||||
|
|
|
@ -426,119 +426,115 @@ read_conf_file (const char *filename, const char *directory, size_t dir_len,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
/* Determine the directories we are looking for data in. */
|
/* Determine the directories we are looking for data in. This function should
|
||||||
|
only be called from __gconv_read_conf. */
|
||||||
static void
|
static void
|
||||||
__gconv_get_path (void)
|
__gconv_get_path (void)
|
||||||
{
|
{
|
||||||
struct path_elem *result;
|
struct path_elem *result;
|
||||||
__libc_lock_define_initialized (static, lock);
|
|
||||||
|
|
||||||
__libc_lock_lock (lock);
|
/* This function is only ever called when __gconv_path_elem is NULL. */
|
||||||
|
result = __gconv_path_elem;
|
||||||
|
assert (result == NULL);
|
||||||
|
|
||||||
/* Make sure there wasn't a second thread doing it already. */
|
/* Determine the complete path first. */
|
||||||
result = (struct path_elem *) __gconv_path_elem;
|
char *gconv_path;
|
||||||
if (result == NULL)
|
size_t gconv_path_len;
|
||||||
|
char *elem;
|
||||||
|
char *oldp;
|
||||||
|
char *cp;
|
||||||
|
int nelems;
|
||||||
|
char *cwd;
|
||||||
|
size_t cwdlen;
|
||||||
|
|
||||||
|
if (__gconv_path_envvar == NULL)
|
||||||
{
|
{
|
||||||
/* Determine the complete path first. */
|
/* No user-defined path. Make a modifiable copy of the
|
||||||
char *gconv_path;
|
default path. */
|
||||||
size_t gconv_path_len;
|
gconv_path = strdupa (default_gconv_path);
|
||||||
char *elem;
|
gconv_path_len = sizeof (default_gconv_path);
|
||||||
char *oldp;
|
cwd = NULL;
|
||||||
char *cp;
|
cwdlen = 0;
|
||||||
int nelems;
|
}
|
||||||
char *cwd;
|
else
|
||||||
size_t cwdlen;
|
{
|
||||||
|
/* Append the default path to the user-defined path. */
|
||||||
|
size_t user_len = strlen (__gconv_path_envvar);
|
||||||
|
|
||||||
if (__gconv_path_envvar == NULL)
|
gconv_path_len = user_len + 1 + sizeof (default_gconv_path);
|
||||||
{
|
gconv_path = alloca (gconv_path_len);
|
||||||
/* No user-defined path. Make a modifiable copy of the
|
__mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar,
|
||||||
default path. */
|
user_len),
|
||||||
gconv_path = strdupa (default_gconv_path);
|
":", 1),
|
||||||
gconv_path_len = sizeof (default_gconv_path);
|
default_gconv_path, sizeof (default_gconv_path));
|
||||||
cwd = NULL;
|
cwd = __getcwd (NULL, 0);
|
||||||
cwdlen = 0;
|
cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd);
|
||||||
}
|
}
|
||||||
else
|
assert (default_gconv_path[0] == '/');
|
||||||
{
|
|
||||||
/* Append the default path to the user-defined path. */
|
|
||||||
size_t user_len = strlen (__gconv_path_envvar);
|
|
||||||
|
|
||||||
gconv_path_len = user_len + 1 + sizeof (default_gconv_path);
|
/* In a first pass we calculate the number of elements. */
|
||||||
gconv_path = alloca (gconv_path_len);
|
oldp = NULL;
|
||||||
__mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar,
|
cp = strchr (gconv_path, ':');
|
||||||
user_len),
|
nelems = 1;
|
||||||
":", 1),
|
while (cp != NULL)
|
||||||
default_gconv_path, sizeof (default_gconv_path));
|
{
|
||||||
cwd = __getcwd (NULL, 0);
|
if (cp != oldp + 1)
|
||||||
cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd);
|
++nelems;
|
||||||
}
|
oldp = cp;
|
||||||
assert (default_gconv_path[0] == '/');
|
cp = strchr (cp + 1, ':');
|
||||||
|
|
||||||
/* In a first pass we calculate the number of elements. */
|
|
||||||
oldp = NULL;
|
|
||||||
cp = strchr (gconv_path, ':');
|
|
||||||
nelems = 1;
|
|
||||||
while (cp != NULL)
|
|
||||||
{
|
|
||||||
if (cp != oldp + 1)
|
|
||||||
++nelems;
|
|
||||||
oldp = cp;
|
|
||||||
cp = strchr (cp + 1, ':');
|
|
||||||
}
|
|
||||||
|
|
||||||
/* Allocate the memory for the result. */
|
|
||||||
result = (struct path_elem *) malloc ((nelems + 1)
|
|
||||||
* sizeof (struct path_elem)
|
|
||||||
+ gconv_path_len + nelems
|
|
||||||
+ (nelems - 1) * (cwdlen + 1));
|
|
||||||
if (result != NULL)
|
|
||||||
{
|
|
||||||
char *strspace = (char *) &result[nelems + 1];
|
|
||||||
int n = 0;
|
|
||||||
|
|
||||||
/* Separate the individual parts. */
|
|
||||||
__gconv_max_path_elem_len = 0;
|
|
||||||
elem = __strtok_r (gconv_path, ":", &gconv_path);
|
|
||||||
assert (elem != NULL);
|
|
||||||
do
|
|
||||||
{
|
|
||||||
result[n].name = strspace;
|
|
||||||
if (elem[0] != '/')
|
|
||||||
{
|
|
||||||
assert (cwd != NULL);
|
|
||||||
strspace = __mempcpy (strspace, cwd, cwdlen);
|
|
||||||
*strspace++ = '/';
|
|
||||||
}
|
|
||||||
strspace = __stpcpy (strspace, elem);
|
|
||||||
if (strspace[-1] != '/')
|
|
||||||
*strspace++ = '/';
|
|
||||||
|
|
||||||
result[n].len = strspace - result[n].name;
|
|
||||||
if (result[n].len > __gconv_max_path_elem_len)
|
|
||||||
__gconv_max_path_elem_len = result[n].len;
|
|
||||||
|
|
||||||
*strspace++ = '\0';
|
|
||||||
++n;
|
|
||||||
}
|
|
||||||
while ((elem = __strtok_r (NULL, ":", &gconv_path)) != NULL);
|
|
||||||
|
|
||||||
result[n].name = NULL;
|
|
||||||
result[n].len = 0;
|
|
||||||
}
|
|
||||||
|
|
||||||
__gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem;
|
|
||||||
|
|
||||||
free (cwd);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
__libc_lock_unlock (lock);
|
/* Allocate the memory for the result. */
|
||||||
|
result = malloc ((nelems + 1)
|
||||||
|
* sizeof (struct path_elem)
|
||||||
|
+ gconv_path_len + nelems
|
||||||
|
+ (nelems - 1) * (cwdlen + 1));
|
||||||
|
if (result != NULL)
|
||||||
|
{
|
||||||
|
char *strspace = (char *) &result[nelems + 1];
|
||||||
|
int n = 0;
|
||||||
|
|
||||||
|
/* Separate the individual parts. */
|
||||||
|
__gconv_max_path_elem_len = 0;
|
||||||
|
elem = __strtok_r (gconv_path, ":", &gconv_path);
|
||||||
|
assert (elem != NULL);
|
||||||
|
do
|
||||||
|
{
|
||||||
|
result[n].name = strspace;
|
||||||
|
if (elem[0] != '/')
|
||||||
|
{
|
||||||
|
assert (cwd != NULL);
|
||||||
|
strspace = __mempcpy (strspace, cwd, cwdlen);
|
||||||
|
*strspace++ = '/';
|
||||||
|
}
|
||||||
|
strspace = __stpcpy (strspace, elem);
|
||||||
|
if (strspace[-1] != '/')
|
||||||
|
*strspace++ = '/';
|
||||||
|
|
||||||
|
result[n].len = strspace - result[n].name;
|
||||||
|
if (result[n].len > __gconv_max_path_elem_len)
|
||||||
|
__gconv_max_path_elem_len = result[n].len;
|
||||||
|
|
||||||
|
*strspace++ = '\0';
|
||||||
|
++n;
|
||||||
|
}
|
||||||
|
while ((elem = __strtok_r (NULL, ":", &gconv_path)) != NULL);
|
||||||
|
|
||||||
|
result[n].name = NULL;
|
||||||
|
result[n].len = 0;
|
||||||
|
}
|
||||||
|
|
||||||
|
__gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem;
|
||||||
|
|
||||||
|
free (cwd);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
/* Read all configuration files found in the user-specified and the default
|
/* Read all configuration files found in the user-specified and the default
|
||||||
path. */
|
path. This function should only be called once during the program's
|
||||||
void
|
lifetime. It disregards locking and synchronization because its only
|
||||||
attribute_hidden
|
caller, __gconv_load_conf, handles this. */
|
||||||
|
static void
|
||||||
__gconv_read_conf (void)
|
__gconv_read_conf (void)
|
||||||
{
|
{
|
||||||
void *modules = NULL;
|
void *modules = NULL;
|
||||||
|
@ -609,6 +605,20 @@ __gconv_read_conf (void)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
/* This "once" variable is used to do a one-time load of the configuration. */
|
||||||
|
__libc_once_define (static, once);
|
||||||
|
|
||||||
|
|
||||||
|
/* Read all configuration files found in the user-specified and the default
|
||||||
|
path, but do it only "once" using __gconv_read_conf to do the actual
|
||||||
|
work. This is the function that must be called when reading iconv
|
||||||
|
configuration. */
|
||||||
|
void
|
||||||
|
__gconv_load_conf (void)
|
||||||
|
{
|
||||||
|
__libc_once (once, __gconv_read_conf);
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
/* Free all resources if necessary. */
|
/* Free all resources if necessary. */
|
||||||
libc_freeres_fn (free_mem)
|
libc_freeres_fn (free_mem)
|
||||||
|
|
|
@ -687,10 +687,6 @@ find_derivation (const char *toset, const char *toset_expand,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
/* Control of initialization. */
|
|
||||||
__libc_once_define (static, once);
|
|
||||||
|
|
||||||
|
|
||||||
static const char *
|
static const char *
|
||||||
do_lookup_alias (const char *name)
|
do_lookup_alias (const char *name)
|
||||||
{
|
{
|
||||||
|
@ -709,7 +705,7 @@ __gconv_compare_alias (const char *name1, const char *name2)
|
||||||
int result;
|
int result;
|
||||||
|
|
||||||
/* Ensure that the configuration data is read. */
|
/* Ensure that the configuration data is read. */
|
||||||
__libc_once (once, __gconv_read_conf);
|
__gconv_load_conf ();
|
||||||
|
|
||||||
if (__gconv_compare_alias_cache (name1, name2, &result) != 0)
|
if (__gconv_compare_alias_cache (name1, name2, &result) != 0)
|
||||||
result = strcmp (do_lookup_alias (name1) ?: name1,
|
result = strcmp (do_lookup_alias (name1) ?: name1,
|
||||||
|
@ -729,7 +725,7 @@ __gconv_find_transform (const char *toset, const char *fromset,
|
||||||
int result;
|
int result;
|
||||||
|
|
||||||
/* Ensure that the configuration data is read. */
|
/* Ensure that the configuration data is read. */
|
||||||
__libc_once (once, __gconv_read_conf);
|
__gconv_load_conf ();
|
||||||
|
|
||||||
/* Acquire the lock. */
|
/* Acquire the lock. */
|
||||||
__libc_lock_lock (__gconv_lock);
|
__libc_lock_lock (__gconv_lock);
|
||||||
|
|
|
@ -178,8 +178,8 @@ extern int __gconv_compare_alias_cache (const char *name1, const char *name2,
|
||||||
extern void __gconv_release_step (struct __gconv_step *step)
|
extern void __gconv_release_step (struct __gconv_step *step)
|
||||||
attribute_hidden;
|
attribute_hidden;
|
||||||
|
|
||||||
/* Read all the configuration data and cache it. */
|
/* Read all the configuration data and cache it if not done so already. */
|
||||||
extern void __gconv_read_conf (void) attribute_hidden;
|
extern void __gconv_load_conf (void) attribute_hidden;
|
||||||
|
|
||||||
/* Try to read module cache file. */
|
/* Try to read module cache file. */
|
||||||
extern int __gconv_load_cache (void) attribute_hidden;
|
extern int __gconv_load_cache (void) attribute_hidden;
|
||||||
|
|
|
@ -0,0 +1,142 @@
|
||||||
|
/* Test that iconv works in a multi-threaded program.
|
||||||
|
Copyright (C) 2017 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, see
|
||||||
|
<http://www.gnu.org/licenses/>. */
|
||||||
|
|
||||||
|
/* This test runs several worker threads that perform the following three
|
||||||
|
steps in staggered synchronization with each other:
|
||||||
|
1. initialization (iconv_open)
|
||||||
|
2. conversion (iconv)
|
||||||
|
3. cleanup (iconv_close)
|
||||||
|
Having several threads synchronously (using pthread_barrier_*) perform
|
||||||
|
these routines exercises iconv code that handles concurrent attempts to
|
||||||
|
initialize and/or read global iconv state (namely configuration). */
|
||||||
|
|
||||||
|
#include <iconv.h>
|
||||||
|
#include <stdio.h>
|
||||||
|
#include <string.h>
|
||||||
|
|
||||||
|
#include <support/support.h>
|
||||||
|
#include <support/xthread.h>
|
||||||
|
#include <support/check.h>
|
||||||
|
|
||||||
|
#define TCOUNT 16
|
||||||
|
_Static_assert (TCOUNT %2 == 0,
|
||||||
|
"thread count must be even, since we need two groups.");
|
||||||
|
|
||||||
|
|
||||||
|
#define CONV_INPUT "Hello, iconv!"
|
||||||
|
|
||||||
|
|
||||||
|
pthread_barrier_t sync;
|
||||||
|
pthread_barrier_t sync_half_pool;
|
||||||
|
|
||||||
|
|
||||||
|
/* Execute iconv_open, iconv and iconv_close in a synchronized way in
|
||||||
|
conjunction with other sibling worker threads. If any step fails, print
|
||||||
|
an error to stdout and return NULL to the main thread to indicate the
|
||||||
|
error. */
|
||||||
|
static void *
|
||||||
|
worker (void * arg)
|
||||||
|
{
|
||||||
|
long int tidx = (long int) arg;
|
||||||
|
|
||||||
|
iconv_t cd;
|
||||||
|
|
||||||
|
char ascii[] = CONV_INPUT;
|
||||||
|
char *inbufpos = ascii;
|
||||||
|
size_t inbytesleft = sizeof (CONV_INPUT);
|
||||||
|
|
||||||
|
char *utf8 = xcalloc (sizeof (CONV_INPUT), 1);
|
||||||
|
char *outbufpos = utf8;
|
||||||
|
size_t outbytesleft = sizeof (CONV_INPUT);
|
||||||
|
|
||||||
|
if (tidx < TCOUNT/2)
|
||||||
|
/* The first half of the worker thread pool synchronize together here,
|
||||||
|
then call iconv_open immediately after. */
|
||||||
|
xpthread_barrier_wait (&sync_half_pool);
|
||||||
|
else
|
||||||
|
/* The second half wait for the first half to finish iconv_open and
|
||||||
|
continue to the next barrier (before the call to iconv below). */
|
||||||
|
xpthread_barrier_wait (&sync);
|
||||||
|
|
||||||
|
/* The above block of code staggers all subsequent pthread_barrier_wait
|
||||||
|
calls in a way that ensures a high chance of encountering these
|
||||||
|
combinations of concurrent iconv usage:
|
||||||
|
1) concurrent calls to iconv_open,
|
||||||
|
2) concurrent calls to iconv_open *and* iconv,
|
||||||
|
3) concurrent calls to iconv,
|
||||||
|
4) concurrent calls to iconv *and* iconv_close,
|
||||||
|
5) concurrent calls to iconv_close. */
|
||||||
|
|
||||||
|
cd = iconv_open ("UTF8", "ASCII");
|
||||||
|
TEST_VERIFY_EXIT (cd != (iconv_t) -1);
|
||||||
|
|
||||||
|
xpthread_barrier_wait (&sync);
|
||||||
|
|
||||||
|
TEST_VERIFY_EXIT (iconv (cd, &inbufpos, &inbytesleft, &outbufpos,
|
||||||
|
&outbytesleft)
|
||||||
|
!= (size_t) -1);
|
||||||
|
|
||||||
|
*outbufpos = '\0';
|
||||||
|
|
||||||
|
xpthread_barrier_wait (&sync);
|
||||||
|
|
||||||
|
TEST_VERIFY_EXIT (iconv_close (cd) == 0);
|
||||||
|
|
||||||
|
/* The next conditional barrier wait is needed because we staggered the
|
||||||
|
threads into two groups in the beginning and at this point, the second
|
||||||
|
half of worker threads are waiting for the first half to finish
|
||||||
|
iconv_close before they can executing the same: */
|
||||||
|
if (tidx < TCOUNT/2)
|
||||||
|
xpthread_barrier_wait (&sync);
|
||||||
|
|
||||||
|
if (strncmp (utf8, CONV_INPUT, sizeof CONV_INPUT))
|
||||||
|
{
|
||||||
|
printf ("FAIL: thread %lx: invalid conversion output from iconv\n", tidx);
|
||||||
|
pthread_exit ((void *) (long int) 1);
|
||||||
|
}
|
||||||
|
|
||||||
|
pthread_exit (NULL);
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
static int
|
||||||
|
do_test (void)
|
||||||
|
{
|
||||||
|
pthread_t thread[TCOUNT];
|
||||||
|
void * worker_output;
|
||||||
|
int i;
|
||||||
|
|
||||||
|
xpthread_barrier_init (&sync, NULL, TCOUNT);
|
||||||
|
xpthread_barrier_init (&sync_half_pool, NULL, TCOUNT/2);
|
||||||
|
|
||||||
|
for (i = 0; i < TCOUNT; i++)
|
||||||
|
thread[i] = xpthread_create (NULL, worker, (void *) (long int) i);
|
||||||
|
|
||||||
|
for (i = 0; i < TCOUNT; i++)
|
||||||
|
{
|
||||||
|
worker_output = xpthread_join (thread[i]);
|
||||||
|
TEST_VERIFY_EXIT (worker_output == NULL);
|
||||||
|
}
|
||||||
|
|
||||||
|
xpthread_barrier_destroy (&sync);
|
||||||
|
xpthread_barrier_destroy (&sync_half_pool);
|
||||||
|
|
||||||
|
return 0;
|
||||||
|
}
|
||||||
|
|
||||||
|
#include <support/test-driver.c>
|
Loading…
Reference in New Issue