From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 0C4B23844035 for ; Tue, 1 Jun 2021 08:32:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0C4B23844035 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-512-zT0OEf6APC2tT3D7XA__TA-1; Tue, 01 Jun 2021 04:32:19 -0400 X-MC-Unique: zT0OEf6APC2tT3D7XA__TA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 32D94106BB29; Tue, 1 Jun 2021 08:32:18 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-113-228.ams2.redhat.com [10.36.113.228]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5ABF660E3A; Tue, 1 Jun 2021 08:32:17 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella via Libc-alpha Subject: Re: [PATCH v2 2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511) References: <20210527172823.3461314-1-adhemerval.zanella@linaro.org> <20210527172823.3461314-3-adhemerval.zanella@linaro.org> Date: Tue, 01 Jun 2021 10:32:15 +0200 In-Reply-To: <20210527172823.3461314-3-adhemerval.zanella@linaro.org> (Adhemerval Zanella via Libc-alpha's message of "Thu, 27 May 2021 14:28:16 -0300") Message-ID: <87r1hm6l4g.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, 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: Tue, 01 Jun 2021 08:32:22 -0000 * Adhemerval Zanella via Libc-alpha: > @@ -424,17 +409,22 @@ start_thread (void *arg) > have ownership (see CONCURRENCY NOTES above). */ > if (__glibc_unlikely (pd->stopped_start)) > { > /* Get the lock the parent locked to force synchronization. */ > lll_lock (pd->lock, LLL_PRIVATE); > > /* We have ownership of PD now. */ > + if (pd->setup_failed == 1) > + { > + /* In the case of a failed setup, the created thread will be > + responsible to free up the allocated resources. The > + detached mode ensure it won't be joined and it will trigger > + the required cleanup. */ > + pd->joinid = pd; > + __do_cancel (); > + } > > /* And give it up right away. */ > lll_unlock (pd->lock, LLL_PRIVATE); > } I don't think this is quite right. The failed thread should not linger around after pthread_create has reported failure to the caller. It's better than what we had before, so maybe we should use this version and clean it up further later. At the point of the __do_cancel call, unwinding may not yet initialized, so the implied dlopen happens on a potentially very small stack. It should be possible to replace it with a goto to the cleanup code. Or maybe put the cleanup code into a separate function and call it here and on the regular cleanup path. Apart from that, it looks fine to me. Thanks, Florian