From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23384 invoked by alias); 1 Sep 2015 12:52:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 23354 invoked by uid 89); 1 Sep 2015 12:51:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_20,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-wi0-f179.google.com Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 01 Sep 2015 12:51:58 +0000 Received: by wicmc4 with SMTP id mc4so31978549wic.0 for ; Tue, 01 Sep 2015 05:51:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to:cc :content-type; bh=6OPMq4EAOVFsODLQ5ycpWFCEBxrvkPSQ5n/ydeTzxrA=; b=SgSOZGBgvkygB3Z06oZNVPpUBzbu5d6u1X/PH3v+GYWsXKSDst2oudtokkzRXqcSw6 jpya+V3F/+AzNujkQKgseGoDHPszCZBn8jzFG4rd3zco83HAJ1gRye48/WZoCM3GrDp9 GMVn5KNM+8PedcCIpkkfIVyvR1HQkTXdmW73+5o36MjrcmaSfByAp6qHwvj6c+nTAFJO GlX1BJgtaVCh9LFyc7a/yejYdJwF9GKl3Vg3K/pAMNrBca4h3rFsGyAga7qV0VuSW2DM XcxNW6dmnrqKs7/79FFJH2BmSSuPBKoHB40JOyFoqyR8HHztI+TRZ7YJojzRbWKMNa0c i+Lw== X-Gm-Message-State: ALoCoQnrFfmHJxQMDjZTraUkzsQqH2/U+EZlW6LW7wtmG/uWLlvwRAyQ+exhMT3OaJdRK5BWWfwI X-Received: by 10.180.206.176 with SMTP id lp16mr3154163wic.85.1441111914979; Tue, 01 Sep 2015 05:51:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.64.193 with HTTP; Tue, 1 Sep 2015 05:51:35 -0700 (PDT) From: Dmitry Vyukov Date: Tue, 01 Sep 2015 12:52:00 -0000 Message-ID: Subject: [Patch, libstdc++] Fix data races in basic_string implementation To: GCC Patches , libstdc++@gcc.gnu.org Cc: Alexander Potapenko , Kostya Serebryany Content-Type: multipart/mixed; boundary=001a11c37be0c6848d051eaf03d4 X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00042.txt.bz2 --001a11c37be0c6848d051eaf03d4 Content-Type: text/plain; charset=UTF-8 Content-length: 6272 Hello, The refcounted basic_string implementation contains several data races on _M_refcount: 1. _M_is_leaked loads _M_refcount concurrently with mutations of _M_refcount. This loads needs to be memory_order_relaxed load, as _M_is_leaked predicate does not change under the feet. 2. _M_is_shared loads _M_refcount concurrently with mutations of _M_refcount. This loads needs to be memory_order_acquire, as another thread can drop _M_refcount to zero concurrently which makes the string non-shared and so the current thread can mutate the string. We need reads of the string in another thread (while it was shared) to happen-before the writes to the string in this thread (now that it is non-shared). This patch adds __gnu_cxx::__atomic_load_dispatch function to do the loads of _M_refcount. The function does an acquire load. Acquire is non needed for _M_is_leaked, but for simplicity as still do acquire (hopefully the refcounted impl will go away in future). This patch also uses the new function to do loads of _M_refcount in string implementation. I did non update doc/xml/manual/concurrency_extensions.xml to document __gnu_cxx::__atomic_load_dispatch, because I am not sure we want to extend that api now that we have language-level atomics. If you still want me to update it, please say how to regenerate doc/html/manual/ext_concurrency.html. The race was detected with ThreadSanitizer on the following program: #define _GLIBCXX_USE_CXX11_ABI 0 #include #include #include #include int main() { std::string s = "foo"; std::thread t([=](){ std::string x = s; std::cout << &x << std::endl; }); std::this_thread::sleep_for(std::chrono::seconds(1)); std::cout << &s[0] << std::endl; t.join(); } $ g++ string.cc -std=c++11 -pthread -O1 -g -fsanitize=thread $ ./a.out WARNING: ThreadSanitizer: data race (pid=98135) Read of size 4 at 0x7d080000efd0 by main thread: #0 std::string::_Rep::_M_is_leaked() const include/c++/6.0.0/bits/basic_string.h:2605 (a.out+0x000000401d7c) #1 std::string::_M_leak() include/c++/6.0.0/bits/basic_string.h:2730 (a.out+0x000000401d7c) #2 std::string::operator[](unsigned long) include/c++/6.0.0/bits/basic_string.h:3274 (a.out+0x000000401d7c) #3 main /tmp/string.cc:14 (a.out+0x000000401d7c) Previous atomic write of size 4 at 0x7d080000efd0 by thread T1: #0 __tsan_atomic32_fetch_add ../../../../libsanitizer/tsan/tsan_interface_atomic.cc:611 (libtsan.so.0+0x00000005811f) #1 __exchange_and_add include/c++/6.0.0/ext/atomicity.h:59 (a.out+0x000000401a19) #2 __exchange_and_add_dispatch include/c++/6.0.0/ext/atomicity.h:92 (a.out+0x000000401a19) #3 std::string::_Rep::_M_dispose(std::allocator const&) include/c++/6.0.0/bits/basic_string.h:2659 (a.out+0x000000401a19) #4 std::basic_string, std::allocator >::~basic_string() include/c++/6.0.0/bits/basic_string.h:2961 (a.out+0x000000401a19) #5 ~ /tmp/string.cc:9 (a.out+0x000000401a19) #6 ~_Head_base include/c++/6.0.0/tuple:102 (a.out+0x000000401a19) #7 ~_Tuple_impl include/c++/6.0.0/tuple:338 (a.out+0x000000401a19) #8 ~tuple include/c++/6.0.0/tuple:521 (a.out+0x000000401a19) #9 ~_Bind_simple include/c++/6.0.0/functional:1503 (a.out+0x000000401a19) #10 ~_Impl include/c++/6.0.0/thread:107 (a.out+0x000000401a19) #11 destroy()> > > include/c++/6.0.0/ext/new_allocator.h:124 (a.out+0x0000004015c7) #12 _S_destroy()> > >, std::thread::_Impl()> > > include/c++/6.0.0/bits/alloc_traits.h:236 (a.out+0x0000004015c7) #13 destroy()> > > include/c++/6.0.0/bits/alloc_traits.h:336 (a.out+0x0000004015c7) #14 _M_dispose include/c++/6.0.0/bits/shared_ptr_base.h:529 (a.out+0x0000004015c7) #15 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:150 (libstdc++.so.6+0x0000000b6da4) #16 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:662 (libstdc++.so.6+0x0000000b6da4) #17 std::__shared_ptr::~__shared_ptr() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:928 (libstdc++.so.6+0x0000000b6da4) #18 std::shared_ptr::~shared_ptr() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr.h:93 (libstdc++.so.6+0x0000000b6da4) #19 execute_native_thread_routine libstdc++-v3/src/c++11/thread.cc:79 (libstdc++.so.6+0x0000000b6da4) Location is heap block of size 28 at 0x7d080000efc0 allocated by main thread: #0 operator new(unsigned long) ../../../../libsanitizer/tsan/tsan_interceptors.cc:571 (libtsan.so.0+0x0000000255a3) #1 __gnu_cxx::new_allocator::allocate(unsigned long, void const*) /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:104 (libstdc++.so.6+0x0000000cbaa8) #2 std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:1051 (libstdc++.so.6+0x0000000cbaa8) #3 __libc_start_main (libc.so.6+0x000000021ec4) Thread T1 (tid=98137, finished) created by main thread at: #0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:895 (libtsan.so.0+0x000000026d04) #1 __gthread_create /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0x0000000b6e52) #2 std::thread::_M_start_thread(std::shared_ptr, void (*)()) libstdc++-v3/src/c++11/thread.cc:149 (libstdc++.so.6+0x0000000b6e52) #3 __libc_start_main (libc.so.6+0x000000021ec4) OK for trunk? --001a11c37be0c6848d051eaf03d4 Content-Type: text/plain; charset=US-ASCII; name="patch.diff" Content-Disposition: attachment; filename="patch.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_ie19d9x60 Content-length: 3567 SW5kZXg6IGluY2x1ZGUvYml0cy9iYXNpY19zdHJpbmcuaAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBpbmNsdWRlL2JpdHMvYmFzaWNfc3RyaW5nLmgJ KHJldmlzaW9uIDIyNzM2MykKKysrIGluY2x1ZGUvYml0cy9iYXNpY19zdHJp bmcuaAkod29ya2luZyBjb3B5KQpAQCAtMjYwMSwxMSArMjYwMSwxMSBAQAog CiAgICAgICAgIGJvb2wKIAlfTV9pc19sZWFrZWQoKSBjb25zdCBfR0xJQkNY WF9OT0VYQ0VQVAotICAgICAgICB7IHJldHVybiB0aGlzLT5fTV9yZWZjb3Vu dCA8IDA7IH0KKyAgICAgICAgeyByZXR1cm4gX19nbnVfY3h4OjpfX2F0b21p Y19sb2FkX2Rpc3BhdGNoKCZ0aGlzLT5fTV9yZWZjb3VudCkgPCAwOyB9CiAK ICAgICAgICAgYm9vbAogCV9NX2lzX3NoYXJlZCgpIGNvbnN0IF9HTElCQ1hY X05PRVhDRVBUCi0gICAgICAgIHsgcmV0dXJuIHRoaXMtPl9NX3JlZmNvdW50 ID4gMDsgfQorICAgICAgICB7IHJldHVybiBfX2dudV9jeHg6Ol9fYXRvbWlj X2xvYWRfZGlzcGF0Y2goJnRoaXMtPl9NX3JlZmNvdW50KSA+IDA7IH0KIAog ICAgICAgICB2b2lkCiAJX01fc2V0X2xlYWtlZCgpIF9HTElCQ1hYX05PRVhD RVBUCkluZGV4OiBpbmNsdWRlL2V4dC9hdG9taWNpdHkuaAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBpbmNsdWRlL2V4dC9hdG9taWNpdHkuaAkocmV2 aXNpb24gMjI3MzYzKQorKysgaW5jbHVkZS9leHQvYXRvbWljaXR5LmgJKHdv cmtpbmcgY29weSkKQEAgLTM1LDYgKzM1LDE2IEBACiAjaW5jbHVkZSA8Yml0 cy9ndGhyLmg+CiAjaW5jbHVkZSA8Yml0cy9hdG9taWNfd29yZC5oPgogCisv LyBFdmVuIGlmIHRoZSBDUFUgZG9lc24ndCBuZWVkIGEgbWVtb3J5IGJhcnJp ZXIsIHdlIG5lZWQgdG8gZW5zdXJlCisvLyB0aGF0IHRoZSBjb21waWxlciBk b2Vzbid0IHJlb3JkZXIgbWVtb3J5IGFjY2Vzc2VzIGFjcm9zcyB0aGUKKy8v IGJhcnJpZXJzLgorI2lmbmRlZiBfR0xJQkNYWF9SRUFEX01FTV9CQVJSSUVS CisjZGVmaW5lIF9HTElCQ1hYX1JFQURfTUVNX0JBUlJJRVIgX19hdG9taWNf dGhyZWFkX2ZlbmNlIChfX0FUT01JQ19BQ1FVSVJFKQorI2VuZGlmCisjaWZu ZGVmIF9HTElCQ1hYX1dSSVRFX01FTV9CQVJSSUVSCisjZGVmaW5lIF9HTElC Q1hYX1dSSVRFX01FTV9CQVJSSUVSIF9fYXRvbWljX3RocmVhZF9mZW5jZSAo X19BVE9NSUNfUkVMRUFTRSkKKyNlbmRpZgorCiBuYW1lc3BhY2UgX19nbnVf Y3h4IF9HTElCQ1hYX1ZJU0lCSUxJVFkoZGVmYXVsdCkKIHsKIF9HTElCQ1hY X0JFR0lOX05BTUVTUEFDRV9WRVJTSU9OCkBAIC01MCw3ICs2MCw3IEBACiAK ICAgc3RhdGljIGlubGluZSB2b2lkCiAgIF9fYXRvbWljX2FkZCh2b2xhdGls ZSBfQXRvbWljX3dvcmQqIF9fbWVtLCBpbnQgX192YWwpCi0gIHsgX19hdG9t aWNfZmV0Y2hfYWRkKF9fbWVtLCBfX3ZhbCwgX19BVE9NSUNfQUNRX1JFTCk7 IH0KKyAgeyBfX2F0b21pY19mZXRjaF9hZGQoX19tZW0sIF9fdmFsLCBfX0FU T01JQ19SRUxFQVNFKTsgfQogI2Vsc2UKICAgX0F0b21pY193b3JkCiAgIF9f YXR0cmlidXRlX18gKChfX3VudXNlZF9fKSkKQEAgLTEwMSwxNyArMTExLDI3 IEBACiAjZW5kaWYKICAgfQogCisgIHN0YXRpYyBpbmxpbmUgX0F0b21pY193 b3JkCisgIF9fYXR0cmlidXRlX18gKChfX3VudXNlZF9fKSkKKyAgX19hdG9t aWNfbG9hZF9kaXNwYXRjaChjb25zdCBfQXRvbWljX3dvcmQqIF9fbWVtKQor ICB7CisjaWZkZWYgX19HVEhSRUFEUworICAgIGlmIChfX2d0aHJlYWRfYWN0 aXZlX3AoKSkKKyAgICAgIHsKKyNpZmRlZiBfR0xJQkNYWF9BVE9NSUNfQlVJ TFRJTlMKKyAgICAgICAgcmV0dXJuIF9fYXRvbWljX2xvYWRfbihfX21lbSwg X19BVE9NSUNfQUNRVUlSRSk7CisjZWxzZQorICAgICAgICAvLyBUaGUgYmVz dCB3ZSBjYW4gZ2V0IHdpdGggYW4gb2xkIGNvbXBpbGVyLgorICAgICAgICBf QXRvbWljX3dvcmQgdiA9ICoodm9sYXRpbGUgX0F0b21pY193b3JkKilfX21l bTsKKyAgICAgICAgX0dMSUJDWFhfUkVBRF9NRU1fQkFSUklFUjsKKyAgICAg ICAgcmV0dXJuIHY7CisjZW5kaWYKKyAgICAgIH0KKyNlbmRpZgorICAgIHJl dHVybiAqX19tZW07CisgIH0KKwogX0dMSUJDWFhfRU5EX05BTUVTUEFDRV9W RVJTSU9OCiB9IC8vIG5hbWVzcGFjZQogCi0vLyBFdmVuIGlmIHRoZSBDUFUg ZG9lc24ndCBuZWVkIGEgbWVtb3J5IGJhcnJpZXIsIHdlIG5lZWQgdG8gZW5z dXJlCi0vLyB0aGF0IHRoZSBjb21waWxlciBkb2Vzbid0IHJlb3JkZXIgbWVt b3J5IGFjY2Vzc2VzIGFjcm9zcyB0aGUKLS8vIGJhcnJpZXJzLgotI2lmbmRl ZiBfR0xJQkNYWF9SRUFEX01FTV9CQVJSSUVSCi0jZGVmaW5lIF9HTElCQ1hY X1JFQURfTUVNX0JBUlJJRVIgX19hdG9taWNfdGhyZWFkX2ZlbmNlIChfX0FU T01JQ19BQ1FVSVJFKQotI2VuZGlmCi0jaWZuZGVmIF9HTElCQ1hYX1dSSVRF X01FTV9CQVJSSUVSCi0jZGVmaW5lIF9HTElCQ1hYX1dSSVRFX01FTV9CQVJS SUVSIF9fYXRvbWljX3RocmVhZF9mZW5jZSAoX19BVE9NSUNfUkVMRUFTRSkK LSNlbmRpZgotCiAjZW5kaWYgCg== --001a11c37be0c6848d051eaf03d4--