From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx-relay69-hz1.antispameurope.com (mx-relay69-hz1.antispameurope.com [94.100.132.235]) by sourceware.org (Postfix) with ESMTPS id 81E183836C3C for ; Thu, 11 Mar 2021 10:38:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 81E183836C3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=net-b.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=prvs=06977f9bf4=burnus@net-b.de Received: from s041.wsp.plusnet.de ([195.90.7.81]) by mx-relay69-hz1.antispameurope.com; Thu, 11 Mar 2021 11:38:54 +0100 Received: from [172.30.2.138] (nat-wv.mentorg.com [192.94.38.34]) by s041.wsp.plusnet.de (Postfix) with ESMTPSA id 1C4A62C034A; Thu, 11 Mar 2021 11:38:48 +0100 (CET) Subject: Re: [Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529] From: Tobias Burnus To: gcc-patches , fortran , Jerry DeLisle , mscfd@gmx.net References: Message-ID: <9b6fa121-a5ea-4d2c-9924-fcdfe7d6d560@net-b.de> Date: Thu, 11 Mar 2021 11:38:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------0C5FAF1979D5610FBF395B11" Content-Language: en-US X-cloud-security-sender: burnus@net-b.de X-cloud-security-recipient: fortran@gcc.gnu.org X-cloud-security-Virusscan: CLEAN X-cloud-security-disclaimer: This E-Mail was scanned by E-Mailservice on mx-relay69-hz1.antispameurope.com with 6B82CE3815 X-cloud-security-connect: s041.wsp.plusnet.de[195.90.7.81], TLS=1, IP=195.90.7.81 X-cloud-security-Digest: cdd9e23b2a518050b42822caff928b22 X-cloud-security: scantime:1.518 X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Mar 2021 10:38:59 -0000 This is a multi-part message in MIME format. --------------0C5FAF1979D5610FBF395B11 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Revised version – the previous had a lock inversion, which could lead to a deadlock, found by -fsanitize=thread. See transfer.c for the changes. OK? Tobias On 11.03.21 10:42, Tobias Burnus wrote: > Hi all, > > as found by Martin (thanks!) there is a race for newunit_free. > While that call is within the unitlock for the calls in io/unit.c, > the call in transfer.c did not use locks. > > Additionally, >       unit = get_gfc_unit (dtp->common.unit, do_create); >       set_internal_unit (dtp, unit, kind); > gets first the unit (with proper locking when using the unit number > dtp->common.unit) but then in set_internal_unit it re-sets the > unit number to the same number without locking. That causes > race warnings and if the assignment is not atomic it is a true race. > > OK for mainline? What about GCC 10? > > As Martin notes in the email thread and in the PR there are more > race warnings (and likely true race issues). > > Tobias > --------------0C5FAF1979D5610FBF395B11 Content-Type: text/x-patch; charset=UTF-8; name="libgf-race-v2.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="libgf-race-v2.diff" Fortran: Fix libgfortran I/O race with newunit_free [PR99529] libgfortran/ChangeLog: * io/transfer.c (st_read_done_worker, st_write_done_worker): Call unlock_unit here, add unit_lock lock around newunit_free call. (st_read_done, st_write_done): Only call unlock_unit when not calling the worker function. * io/unit.c (set_internal_unit): Don't reset the unit_number to the same number as this cause race warnings. diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c index 27bee9d4e01..71a935652e3 100644 --- a/libgfortran/io/transfer.c +++ b/libgfortran/io/transfer.c @@ -4339,6 +4339,7 @@ export_proto(st_read_done); void st_read_done_worker (st_parameter_dt *dtp) { + bool free_newunit = false; finalize_transfer (dtp); free_ionml (dtp); @@ -4358,7 +4359,7 @@ st_read_done_worker (st_parameter_dt *dtp) free (dtp->u.p.current_unit->ls); dtp->u.p.current_unit->ls = NULL; } - newunit_free (dtp->common.unit); + free_newunit = true; } if (dtp->u.p.unit_is_internal || dtp->u.p.format_not_saved) { @@ -4366,6 +4367,14 @@ st_read_done_worker (st_parameter_dt *dtp) free_format (dtp); } } + unlock_unit (dtp->u.p.current_unit); + if (free_newunit) + { + /* Avoid inverse lock issues by placing after unlock_unit. */ + LOCK (&unit_lock); + newunit_free (dtp->common.unit); + UNLOCK (&unit_lock); + } } void @@ -4382,11 +4391,10 @@ st_read_done (st_parameter_dt *dtp) if (dtp->u.p.async) enqueue_done (dtp->u.p.current_unit->au, AIO_READ_DONE); } + unlock_unit (dtp->u.p.current_unit); } else - st_read_done_worker (dtp); - - unlock_unit (dtp->u.p.current_unit); + st_read_done_worker (dtp); /* Calls unlock_unit. */ } library_end (); @@ -4406,6 +4414,7 @@ st_write (st_parameter_dt *dtp) void st_write_done_worker (st_parameter_dt *dtp) { + bool free_newunit = false; finalize_transfer (dtp); if (dtp->u.p.current_unit != NULL @@ -4446,7 +4455,7 @@ st_write_done_worker (st_parameter_dt *dtp) free (dtp->u.p.current_unit->ls); dtp->u.p.current_unit->ls = NULL; } - newunit_free (dtp->common.unit); + free_newunit = true; } if (dtp->u.p.unit_is_internal || dtp->u.p.format_not_saved) { @@ -4454,6 +4463,14 @@ st_write_done_worker (st_parameter_dt *dtp) free_format (dtp); } } + unlock_unit (dtp->u.p.current_unit); + if (free_newunit) + { + /* Avoid inverse lock issues by placing after unlock_unit. */ + LOCK (&unit_lock); + newunit_free (dtp->common.unit); + UNLOCK (&unit_lock); + } } extern void st_write_done (st_parameter_dt *); @@ -4476,11 +4493,10 @@ st_write_done (st_parameter_dt *dtp) if (dtp->u.p.async) enqueue_done (dtp->u.p.current_unit->au, AIO_WRITE_DONE); } + unlock_unit (dtp->u.p.current_unit); } else - st_write_done_worker (dtp); - - unlock_unit (dtp->u.p.current_unit); + st_write_done_worker (dtp); /* Calls unlock_unit. */ } library_end (); diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c index 2ea664331bc..b0cc6ab2301 100644 --- a/libgfortran/io/unit.c +++ b/libgfortran/io/unit.c @@ -456,7 +456,6 @@ set_internal_unit (st_parameter_dt *dtp, gfc_unit *iunit, int kind) { gfc_offset start_record = 0; - iunit->unit_number = dtp->common.unit; iunit->recl = dtp->internal_unit_len; iunit->internal_unit = dtp->internal_unit; iunit->internal_unit_len = dtp->internal_unit_len; --------------0C5FAF1979D5610FBF395B11--