From patchwork Wed Sep 20 13:08:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arjun Shankar X-Patchwork-Id: 816186 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-84781-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="wURhqWBn"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3xy0S15C3Tz9sP1 for ; Wed, 20 Sep 2017 23:08:49 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:subject:message-id:mime-version :content-type; q=dns; s=default; b=Z2dx0EtsJG0tr9LQkgTPVma9Uj3mg QvOW2AcWHj2eytFIe/S1hy59q5CgD+BzY3ccGhqNkixo6x4Vx+Sy9EvUb3ygYg4l WFsdjB9kbfIA4bwppj22dICHp7uwiMB413U1lpgIhOUJfyFxx+GkvjgTvXEBsLCI r9EFImuCu/JNPw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:subject:message-id:mime-version :content-type; s=default; bh=6Gr6Oqe6luh5DVssHOGXjh3pRGc=; b=wUR hqWBn2hUIYtALqLF1kJkFbuEz7QKPtYNkpnv6/gcuaVs7nTIUeKrg3CEh2LY+WDV Wi1SVHW0eDXMPsXRBmgOOaYfDhSTLcGWB7f2rReZOLMQ+iY6TL0EYxRNkuG6U8Ys oqw6dJuYdzA7r7VelAe+XLgpRKmyyhyomuT2gVD8= Received: (qmail 116987 invoked by alias); 20 Sep 2017 13:08:41 -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 116020 invoked by uid 89); 20 Sep 2017 13:08:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=arjun, slash, H*F:D*se X-HELO: aloka.lostca.se Date: Wed, 20 Sep 2017 13:08:36 +0000 From: Arjun Shankar To: libc-alpha@sourceware.org Subject: [PATCH] Fix double-checked locking in __gconv_get_path and __gconv_read_conf [BZ #22062] Message-ID: <20170920130836.GA3716@aloka.lostca.se> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) These two functions running in different threads could encounter a data race if the CPU or compiler reordered the store of __gconv_path_elem so as to be earlier than writes to the content that it points to. This has now been corrected by using atomics every time the variable is accessed. ChangeLog: 2017-09-20 Arjun Shankar * iconv/gconv_conf.c: Include . (__gconv_get_path): Access __gconv_path_elem using atomics. (__gconv_read_conf): Likewise. (libc_freeres_fn): Likewise. --- iconv/gconv_conf.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c index f1c28ce..8a5fa28 100644 --- a/iconv/gconv_conf.c +++ b/iconv/gconv_conf.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -428,8 +429,13 @@ __gconv_get_path (void) __libc_lock_lock (lock); - /* Make sure there wasn't a second thread doing it already. */ - result = (struct path_elem *) __gconv_path_elem; + /* __gconv_read_conf () uses double-checked locking and therefore can make + a redundant call to this function while another thread is already + executing it. So first we make sure another thread has not already done + the work we want to do. + + A relaxed MO load is sufficient since we already have the lock. */ + result = atomic_load_relaxed (&__gconv_path_elem); if (result == NULL) { /* Determine the complete path first. */ @@ -519,7 +525,10 @@ __gconv_get_path (void) result[n].len = 0; } - __gconv_path_elem = result ?: (struct path_elem *) &empty_path_elem; + /* This store synchronizes with the acquire MO load in + __gconv_read_conf (). */ + atomic_store_release (&__gconv_path_elem, + result ?: (struct path_elem *) &empty_path_elem); free (cwd); } @@ -538,6 +547,7 @@ __gconv_read_conf (void) size_t nmodules = 0; int save_errno = errno; size_t cnt; + struct path_elem *gconv_path_elem_local; /* First see whether we should use the cache. */ if (__gconv_load_cache () == 0) @@ -549,13 +559,20 @@ __gconv_read_conf (void) #ifndef STATIC_GCONV /* Find out where we have to look. */ - if (__gconv_path_elem == NULL) - __gconv_get_path (); - for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt) + /* This load is synchronized with the release MO store done during the + initialization of __gconv_path_elem in __gconv_get_path (). */ + gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem); + if (gconv_path_elem_local == NULL) + { + __gconv_get_path (); + gconv_path_elem_local = atomic_load_acquire (&__gconv_path_elem); + } + + for (cnt = 0; gconv_path_elem_local[cnt].name != NULL; ++cnt) { - const char *elem = __gconv_path_elem[cnt].name; - size_t elem_len = __gconv_path_elem[cnt].len; + const char *elem = gconv_path_elem_local[cnt].name; + size_t elem_len = gconv_path_elem_local[cnt].len; char *filename; /* No slash needs to be inserted between elem and gconv_conf_filename; @@ -606,6 +623,11 @@ __gconv_read_conf (void) /* Free all resources if necessary. */ libc_freeres_fn (free_mem) { - if (__gconv_path_elem != NULL && __gconv_path_elem != &empty_path_elem) - free ((void *) __gconv_path_elem); + /* __gconv_path_elem is always accessed atomically because it is used in + double-checked locking. Since this function is called when the process + has become single-threaded, it is enough to used a relaxed MO load. */ + struct path_elem *gconv_path_elem_local + = atomic_load_relaxed (&__gconv_path_elem); + if (gconv_path_elem_local != NULL && gconv_path_elem_local != &empty_path_elem) + free ((void *) gconv_path_elem_local); }