Aw: [PATCH, committed] [9/10/11 Regression] PR fortran/95104 - Segfault on a legal WAIT statement

Message ID trinity-63fcd4d9-bb9a-42d1-bc54-90ae95d659f7-1590691674952@3c-app-gmx-bs20
State New
Headers show
Series
  • Aw: [PATCH, committed] [9/10/11 Regression] PR fortran/95104 - Segfault on a legal WAIT statement
Related show

Commit Message

Harald Anlauf May 28, 2020, 6:47 p.m.
The fix for

> PR fortran/95104 - Segfault on a legal WAIT statement

>

> Referencing a unit in a WAIT statement that has not been opened before

> resulted in a NULL pointer dereference.  Check for this condition.

>

> 2020-05-26  Harald Anlauf  <anlauf@gmx.de>

>

> libgfortran/

> 	PR libfortran/95104

> 	* io/transfer.c (st_wait_async): Do not dereference NULL pointer.

>

> gcc/testsuite/

> 	PR libfortran/95104

> 	* gfortran.dg/pr95104.f90: New test.

>

> Co-Authored-By: Steven G. Kargl  <kargl@gcc.gnu.org>


did uncover a latent issue with regard to unit locking that was introduced in the
context of asynchronous I/O in libgfortran.  This was reported by Rainer Orth, see

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95104#c13

I can reproduce this when compiling/linking with -fopenmp.

There are two possible fixes for this:

(1) guard the call to unlock_unit by:



Does anybody prefer one over the other, or just commit both (which might be
preferable to catch other unguarded cases)?

Thanks,
Harald

Comments

Kees Cook via Gcc-patches May 28, 2020, 6:55 p.m. | #1
Hi Harald,

> There are two possible fixes for this:

> 

> (1) guard the call to unlock_unit by:

> 

> diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c

> index cd51679ff46..296be0711a2 100644

> --- a/libgfortran/io/transfer.c

> +++ b/libgfortran/io/transfer.c

> @@ -4508,7 +4508,8 @@ st_wait_async (st_parameter_wait *wtp)

>          async_wait (&(wtp->common), u->au);

>       }

> 

> -  unlock_unit (u);

> +  if (u)

> +    unlock_unit (u);

>   }

> 

> 

> 

> (2) in unlock_unit():

> 

> diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c

> index 0030d7e8701..a3b0656cb90 100644

> --- a/libgfortran/io/unit.c

> +++ b/libgfortran/io/unit.c

> @@ -767,9 +767,12 @@ close_unit_1 (gfc_unit *u, int locked)

>   void

>   unlock_unit (gfc_unit *u)

>   {

> -  NOTE ("unlock_unit = %d", u->unit_number);

> -  UNLOCK (&u->lock);

> -  NOTE ("unlock_unit done");

> +  if (u)

> +    {

> +      NOTE ("unlock_unit = %d", u->unit_number);

> +      UNLOCK (&u->lock);

> +      NOTE ("unlock_unit done");

> +    }

>   }

> 

>   /* close_unit()-- Close a unit.  The stream is closed, and any memory

> 

> 

> Does anybody prefer one over the other, or just commit both (which might be

> preferable to catch other unguarded cases)?


I think the second one is more robust - like you say, this may catch
other cases.  If we have that one, we don't need the first one.

Regards

	Thomas
Harald Anlauf May 28, 2020, 7:58 p.m. | #2
Dear all,

> I think the second one is more robust - like you say, this may catch

> other cases.  If we have that one, we don't need the first one.


to fix the fallout I've committed to master the following patch,
which I will backport to the affected branches (9/10):


PR fortran/95104 - Segfault on a legal WAIT statement

The initial commit for this PR uncovered a latent issue with unit locking
in the Fortran run-time library.  Add check for valid unit.

2020-05-28  Harald Anlauf  <anlauf@gmx.de>

libgfortran/
	PR libfortran/95104
	* io/unit.c (unlock_unit): Guard by check for NULL pointer.


diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 0030d7e8701..a3b0656cb90 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -767,9 +767,12 @@ close_unit_1 (gfc_unit *u, int locked)
 void
 unlock_unit (gfc_unit *u)
 {
-  NOTE ("unlock_unit = %d", u->unit_number);
-  UNLOCK (&u->lock);
-  NOTE ("unlock_unit done");
+  if (u)
+    {
+      NOTE ("unlock_unit = %d", u->unit_number);
+      UNLOCK (&u->lock);
+      NOTE ("unlock_unit done");
+    }
 }

 /* close_unit()-- Close a unit.  The stream is closed, and any memory
Kees Cook via Gcc-patches May 29, 2020, 4:53 p.m. | #3
Am 28.05.20 um 21:58 schrieb Harald Anlauf:
> The initial commit for this PR uncovered a latent issue with unit locking

> in the Fortran run-time library.  Add check for valid unit.


This only came up because Solaris, unlike Linux, links the pthreads
library by default, so it will not be found in a normal regression
test.

Is there a magic incantation which would let the gfortran testsuite
cycle through -phtread at least once, so this kind of thing can
be found earlier?  Of course, this would have to be restricted
to those platforms which actually support pthreds...

Regards

	Thomas

Patch

diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index cd51679ff46..296be0711a2 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -4508,7 +4508,8 @@  st_wait_async (st_parameter_wait *wtp)
        async_wait (&(wtp->common), u->au);
     }

-  unlock_unit (u);
+  if (u)
+    unlock_unit (u);
 }



(2) in unlock_unit():

diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 0030d7e8701..a3b0656cb90 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -767,9 +767,12 @@  close_unit_1 (gfc_unit *u, int locked)
 void
 unlock_unit (gfc_unit *u)
 {
-  NOTE ("unlock_unit = %d", u->unit_number);
-  UNLOCK (&u->lock);
-  NOTE ("unlock_unit done");
+  if (u)
+    {
+      NOTE ("unlock_unit = %d", u->unit_number);
+      UNLOCK (&u->lock);
+      NOTE ("unlock_unit done");
+    }
 }

 /* close_unit()-- Close a unit.  The stream is closed, and any memory