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

[Tobias Burnus <burnus@net-b.de>]

[-- 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;