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

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

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