From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id 8F853395C872 for ; Wed, 20 May 2020 14:43:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8F853395C872 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-467-Eki5PpiCOfShN2iaHnumOg-1; Wed, 20 May 2020 10:42:59 -0400 X-MC-Unique: Eki5PpiCOfShN2iaHnumOg-1 Received: by mail-qt1-f199.google.com with SMTP id v17so3942195qtp.15 for ; Wed, 20 May 2020 07:42:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=G+PnwYk8SrA1zEo7jzHNylGnhJgYdQUpQTE76Vz1Kew=; b=J167xeqeixcBcKoZba1V+BDcYxuE2swz5+O0MFRHsSMBIB9tF/FWQCN7bPdUnSX+TY FmmsdwL+3iTcSXvnLCAEzW+7KOECY1F11D9aBf+ZrytwnK0WEJgH5A1JRh0eVjGJOIOw iLSp7rkoOa+42UZq/s8pSHri/pbMBuShW/p3MizNs2cvNPjg3WXJ8BEbKBygalayoYH0 w2d4pLbpqjK5M04r6zeS48+33oW2IUvh0DnPkG4kg4kpZ1LSlxJW3WvjIS3LAQSXAShE E8+v9NgfmsCFwY2Wcv3HvtYfiut9OI9Yz0NRP58V9N4jP4mqX2J8BFRVQw5luEkmu2BW LqpQ== X-Gm-Message-State: AOAM533A3D1/W3UdyZECPi6JcMEJLrs2yhbYshxw1WMjK9OHkpfSXDsO I/4QCLxKyENSvmEDAWJRLk7MuEiBDROmJfaDQSPOmbe/jCU9nUY9XJtprYiKtAYbe8dDOeEBG+4 MxkCSmczcamWCLsclF74r X-Received: by 2002:ac8:2fb9:: with SMTP id l54mr5262472qta.211.1589985778953; Wed, 20 May 2020 07:42:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJynxodxP1g0gDPu6xOb5Cj9LjbqN3MomzfNdgeWaQGFEk8DduSwO8J2uRFLNLTQH5SL1xQAPA== X-Received: by 2002:ac8:2fb9:: with SMTP id l54mr5262443qta.211.1589985778649; Wed, 20 May 2020 07:42:58 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id y23sm2848318qta.37.2020.05.20.07.42.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 May 2020 07:42:57 -0700 (PDT) Subject: Re: [PATCH 08/19] nptl: Use __pthread_attr_copy in pthread_getattr_default_np (bug 25999) To: Florian Weimer , libc-alpha@sourceware.org References: From: Carlos O'Donell Organization: Red Hat Message-ID: <986cb140-5a63-51ac-95a4-526a20d0e959@redhat.com> Date: Wed, 20 May 2020 10:42:56 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 May 2020 14:43:08 -0000 On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote: > pthread_getattr_default_np needs to make a deep copy. Agreed, this seems like an oversight regarding the affinity. I expect not enough programs ever use the kind of setup you run in the test, but it's still wrong, we should be doing a deep copy. Tested clean on x86_64 and i686. OK for master. Reviewed-by: Carlos O'Donell Tested-by: Carlos O'Donell > --- > nptl/Makefile | 1 + > nptl/pthread_getattr_default_np.c | 12 ++--- > nptl/tst-pthread-defaultattr-free.c | 78 +++++++++++++++++++++++++++++ > 3 files changed, 82 insertions(+), 9 deletions(-) > create mode 100644 nptl/tst-pthread-defaultattr-free.c > > diff --git a/nptl/Makefile b/nptl/Makefile > index b4aaad0f20..e5686b20ac 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -328,6 +328,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \ > tst-thread-affinity-pthread \ > tst-thread-affinity-pthread2 \ > tst-thread-affinity-sched \ > + tst-pthread-defaultattr-free \ OK. > > > tests-container = tst-pthread-getattr > diff --git a/nptl/pthread_getattr_default_np.c b/nptl/pthread_getattr_default_np.c > index cce20cbe94..a9665c5df7 100644 > --- a/nptl/pthread_getattr_default_np.c > +++ b/nptl/pthread_getattr_default_np.c > @@ -16,20 +16,14 @@ > License along with the GNU C Library; if not, see > . */ > > -#include > -#include > #include > > int > pthread_getattr_default_np (pthread_attr_t *out) > { > - struct pthread_attr *real_out; > - > - real_out = (struct pthread_attr *) out; > - > lll_lock (__default_pthread_attr_lock, LLL_PRIVATE); > - *real_out = __default_pthread_attr; > + int ret = __pthread_attr_copy (out, > + (pthread_attr_t *) &__default_pthread_attr); OK. Locks kept in place, and call to __pthread_attr_copy owns the struct. > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > - > - return 0; > + return ret; > } > diff --git a/nptl/tst-pthread-defaultattr-free.c b/nptl/tst-pthread-defaultattr-free.c > new file mode 100644 > index 0000000000..5a84302380 > --- /dev/null > +++ b/nptl/tst-pthread-defaultattr-free.c > @@ -0,0 +1,78 @@ > +/* Test for user-after-free bug in pthread_getattr_default_np (bug 25999). OK. > + Copyright (C) 2020 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 > + . */ > + > +#include > +#include > +#include > +#include > + > +static int > +do_test (void) > +{ > + /* This is a typical affinity size. */ > + enum { cpu_count = 128 }; > + cpu_set_t *set = CPU_ALLOC (cpu_count); > + size_t set_size = CPU_ALLOC_SIZE (cpu_count); > + CPU_ZERO_S (set_size, set); > + CPU_SET (1, set); > + CPU_SET (3, set); OK. > + > + /* Apply the affinity mask to the default attribute. */ > + pthread_attr_t attr; > + xpthread_attr_init (&attr); > + TEST_COMPARE (pthread_attr_setaffinity_np (&attr, set_size, set), 0); > + TEST_COMPARE (pthread_setattr_default_np (&attr), 0); OK. Set the default. > + xpthread_attr_destroy (&attr); OK. > + > + /* Read back the default attribute and check affinity mask. */ > + pthread_getattr_default_np (&attr); > + CPU_ZERO_S (set_size, set); > + TEST_COMPARE (pthread_attr_getaffinity_np (&attr, set_size, set), 0); > + for (int i = 0; i < cpu_count; ++i) > + TEST_COMPARE (!!CPU_ISSET (i, set), i == 1 || i == 3); OK. > + > + > + /* Apply a larger CPU affinity mask to the default attribute, to > + trigger reallocation. */ > + { > + cpu_set_t *large_set = CPU_ALLOC (4 * cpu_count); > + size_t large_set_size = CPU_ALLOC_SIZE (4 * cpu_count); > + CPU_ZERO_S (large_set_size, large_set); > + pthread_attr_t large_attr; > + xpthread_attr_init (&large_attr); > + TEST_COMPARE (pthread_attr_setaffinity_np (&large_attr, > + large_set_size, large_set), 0); > + TEST_COMPARE (pthread_setattr_default_np (&large_attr), 0); OK. > + xpthread_attr_destroy (&large_attr); > + CPU_FREE (large_set); > + } > + > + /* Read back the default attribute and check affinity mask. */ > + CPU_ZERO_S (set_size, set); > + TEST_COMPARE (pthread_attr_getaffinity_np (&attr, set_size, set), 0); > + for (int i = 0; i < cpu_count; ++i) > + TEST_COMPARE (!!CPU_ISSET (i, set), i == 1 || i == 3); > + /* Due to bug 25999, the following caused a double-free abort. */ > + xpthread_attr_destroy (&attr); OK. > + > + CPU_FREE (set); > + > + return 0; > +} > + > +#include > -- Cheers, Carlos.