From patchwork Tue Oct 16 14:23:41 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arjun Shankar X-Patchwork-Id: 29767 Received: (qmail 113252 invoked by alias); 16 Oct 2018 14:23:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 112854 invoked by uid 89); 16 Oct 2018 14:23:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_PASS autolearn=ham version=3.3.2 spammy=H*F:D*se, internally, do_test X-HELO: aloka.lostca.se DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=lostca.se; h=date:from:to :subject:message-id:mime-version:content-type; s=howrah; bh=TD9j 2Ew1mmQoTrNo2p9Lfxgf+nY=; b=F3GV7zCiT5tOg8I4Tf9HhLSEUgeDOEIhwRmD kr2KKGUBTAUYLHP7BsN+H1n281FwJMxZc5bUK+8G4NOm5MSPW+zewQFmm69eAjji TkD7WGLR8saQdTUT+jFEqfM6LH630gnOyYwXVSRSffci3vH/uEk89j2mLVbwpVqM ZqD7K6Y= Date: Tue, 16 Oct 2018 14:23:41 +0000 From: Arjun Shankar To: libc-alpha@sourceware.org, Carlos O'Donell , Florian Weimer Subject: [PATCH v2] Remove unnecessary locking when reading iconv configuration [BZ #22062] Message-ID: <20181016142341.GA37762@aloka.lostca.se> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) 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. ChangeLog: 2018-10-16 Arjun Shankar [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. --- iconv/Makefile | 5 +- iconv/gconv_conf.c | 208 +++++++++++++++++++++++-------------------- iconv/gconv_db.c | 8 +- iconv/gconv_int.h | 4 +- iconv/tst-iconv-mt.c | 142 +++++++++++++++++++++++++++++ 5 files changed, 259 insertions(+), 108 deletions(-) create mode 100644 iconv/tst-iconv-mt.c This v2 of the patch addresses comments from the review of v1: https://sourceware.org/ml/libc-alpha/2017-10/msg01195.html diff --git a/iconv/Makefile b/iconv/Makefile index d71319b39e..79d03c3bbe 100644 --- a/iconv/Makefile +++ b/iconv/Makefile @@ -43,7 +43,8 @@ CFLAGS-charmap.c += -DCHARMAP_PATH='"$(i18ndir)/charmaps"' \ CFLAGS-linereader.c += -DNO_TRANSLITERATION 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 install-others-programs = $(inst_bindir)/iconv @@ -67,6 +68,8 @@ endif $(objpfx)gconv-modules: test-gconv-modules cp $< $@ +$(objpfx)tst-iconv-mt: $(shared-thread-library) + ifeq (yes,$(build-shared)) tests += tst-gconv-init-failure modules-names += tst-gconv-init-failure-mod diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c index ce9f10f3af..78010491e6 100644 --- a/iconv/gconv_conf.c +++ b/iconv/gconv_conf.c @@ -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 __gconv_get_path (void) { struct path_elem *result; - __libc_lock_define_initialized (static, lock); - __libc_lock_lock (lock); - - /* Make sure there wasn't a second thread doing it already. */ - result = (struct path_elem *) __gconv_path_elem; - if (result == NULL) + /* This function is only ever called when __gconv_path_elem is NULL. */ + result = __gconv_path_elem; + assert (result == NULL); + + /* Determine the complete path first. */ + char *gconv_path; + 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. */ - char *gconv_path; - size_t gconv_path_len; - char *elem; - char *oldp; - char *cp; - int nelems; - char *cwd; - size_t cwdlen; - - if (__gconv_path_envvar == NULL) - { - /* No user-defined path. Make a modifiable copy of the - default path. */ - gconv_path = strdupa (default_gconv_path); - gconv_path_len = sizeof (default_gconv_path); - cwd = NULL; - cwdlen = 0; - } - else - { - /* 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); - gconv_path = alloca (gconv_path_len); - __mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar, - user_len), - ":", 1), - default_gconv_path, sizeof (default_gconv_path)); - cwd = __getcwd (NULL, 0); - cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd); - } - assert (default_gconv_path[0] == '/'); - - /* 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; - } + /* No user-defined path. Make a modifiable copy of the + default path. */ + gconv_path = strdupa (default_gconv_path); + gconv_path_len = sizeof (default_gconv_path); + cwd = NULL; + cwdlen = 0; + } + else + { + /* 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); + gconv_path = alloca (gconv_path_len); + __mempcpy (__mempcpy (__mempcpy (gconv_path, __gconv_path_envvar, + user_len), + ":", 1), + default_gconv_path, sizeof (default_gconv_path)); + cwd = __getcwd (NULL, 0); + cwdlen = __glibc_unlikely (cwd == NULL) ? 0 : strlen (cwd); + } + assert (default_gconv_path[0] == '/'); - __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem; + /* 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, ':'); + } - free (cwd); + /* 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; } - __libc_lock_unlock (lock); + __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem; + + free (cwd); } /* Read all configuration files found in the user-specified and the default - path. */ -void -attribute_hidden + path. This function should only be called once during the program's + lifetime. It disregards locking and synchronization because its only + caller, __gconv_load_conf, handles this. */ +static void __gconv_read_conf (void) { 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. */ libc_freeres_fn (free_mem) diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c index 66e095d8c7..86acfc7d74 100644 --- a/iconv/gconv_db.c +++ b/iconv/gconv_db.c @@ -687,10 +687,6 @@ find_derivation (const char *toset, const char *toset_expand, } -/* Control of initialization. */ -__libc_once_define (static, once); - - static const char * do_lookup_alias (const char *name) { @@ -709,7 +705,7 @@ __gconv_compare_alias (const char *name1, const char *name2) int result; /* 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) result = strcmp (do_lookup_alias (name1) ?: name1, @@ -729,7 +725,7 @@ __gconv_find_transform (const char *toset, const char *fromset, int result; /* Ensure that the configuration data is read. */ - __libc_once (once, __gconv_read_conf); + __gconv_load_conf (); /* Acquire the lock. */ __libc_lock_lock (__gconv_lock); diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h index 45e47a6511..dcdb1bce76 100644 --- a/iconv/gconv_int.h +++ b/iconv/gconv_int.h @@ -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) attribute_hidden; -/* Read all the configuration data and cache it. */ -extern void __gconv_read_conf (void) attribute_hidden; +/* Read all the configuration data and cache it if not done so already. */ +extern void __gconv_load_conf (void) attribute_hidden; /* Try to read module cache file. */ extern int __gconv_load_cache (void) attribute_hidden; diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c new file mode 100644 index 0000000000..aa91a980e5 --- /dev/null +++ b/iconv/tst-iconv-mt.c @@ -0,0 +1,142 @@ +/* Test that iconv works in a multi-threaded program. + Copyright (C) 2018 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 + . */ + +/* 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 +#include +#include + +#include +#include +#include + +#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