[Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

[Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

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


[-- Attachment #2: libgf-race.diff --]
[-- Type: text/x-patch, Size: 1569 bytes --]

Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

libgfortran/ChangeLog:

	* io/transfer.c (st_read_done_worker, st_write_done_worker):
	Add unit_lock lock/unlock around newunit_free call.
	* 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..a6f115d5803 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -4358,7 +4358,9 @@ st_read_done_worker (st_parameter_dt *dtp)
 		free (dtp->u.p.current_unit->ls);
 	      dtp->u.p.current_unit->ls = NULL;
 	    }
+	  LOCK (&unit_lock);
 	  newunit_free (dtp->common.unit);
+	  UNLOCK (&unit_lock);
 	}
       if (dtp->u.p.unit_is_internal || dtp->u.p.format_not_saved)
 	{
@@ -4446,7 +4448,9 @@ st_write_done_worker (st_parameter_dt *dtp)
 		free (dtp->u.p.current_unit->ls);
 	      dtp->u.p.current_unit->ls = NULL;
 	    }
+	  LOCK (&unit_lock);
 	  newunit_free (dtp->common.unit);
+	  UNLOCK (&unit_lock);
 	}
       if (dtp->u.p.unit_is_internal || dtp->u.p.format_not_saved)
 	{
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;

Re: [Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

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
>

[-- Attachment #2: libgf-race-v2.diff --]
[-- Type: text/x-patch, Size: 3528 bytes --]

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;

Re: [Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

[Martin Stein]

I have seen the lock inversion with helgrind as well. Otherwise I
have not seen anything more in my real code. I will give the
thread sanitizer another try. Problem with both are the huge
number of false positives in conjunction with typical openmp code
(e.g. master thread allocates a big vector or matrix outside
openmp region, and then it is accessed within openmp loop by all
threads without locks of course -> possible data race warning).

BTW, I will do some more tests, but it looks like the patch fixes
the memory corruption issue. This morning I tried with a wrong
build setup, as I used patched gfortran but linked with unpatched
gcc, which probably used the unpatched libgfortran...

Thanks!
Martin
 
 
 

Gesendet: Donnerstag, 11. März 2021 um 11:38 Uhr
Von: "Tobias Burnus" <burnus@net-b.de>
An: "gcc-patches" <gcc-patches@gcc.gnu.org>, "fortran" <fortran@gcc.gnu.org>, "Jerry DeLisle" <jvdelisle@charter.net>, mscfd@gmx.net
Betreff: Re: [Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]
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
>

Re: [Patch] Fortran: Fix libgfortran I/O race with newunit_free [PR99529]

[Jerry DeLisle]

Looks good Tobias, OK,

Odd about that line in set_internal_unit. Probably leftover from a 
copy/paste. I see in comment #5 of the PR that you mentioned trying the 
assert to make sure it is useless code.

Thanks for the patch,

Jerry

On 3/11/21 2:38 AM, Tobias Burnus wrote:
> 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
>>