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

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

[Harald Anlauf]

Committed as obvious.

The invalid NULL pointer dereference was discovered by Steve Kargl.

Will backport in a few days, when I figure out how to do it now.

Thanks,
Harald


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>

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

[Thomas Koenig]

Am 26.05.20 um 23:33 schrieb Harald Anlauf:
> Committed as obvious.
> 
> The invalid NULL pointer dereference was discovered by Steve Kargl.
> 
> Will backport in a few days, when I figure out how to do it now.

Thanks for committing this.

The way to backport now is to first run contrib/gcc-git-customization.sh
from current master, and then change to the branch you want to
backport this to and run

git gcc-backport r11-646-g56f03cd12be26828788a27f6f3c250041a958e45 .

(or what your revision may be).

I just tried it, and it works well.

Regards

	Thomas

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

[Harald Anlauf]

Hi Thomas,

thanks for the hint:

> Von: "Thomas Koenig" <tkoenig@netcologne.de>
> Am 26.05.20 um 23:33 schrieb Harald Anlauf:
> > Will backport in a few days, when I figure out how to do it now.
>
> The way to backport now is to first run contrib/gcc-git-customization.sh
> from current master, and then change to the branch you want to
> backport this to and run
>
> git gcc-backport r11-646-g56f03cd12be26828788a27f6f3c250041a958e45 .
>
> (or what your revision may be).
>
> I just tried it, and it works well.

This almost worked for me.

I am using git worktree for the branches.  I had to checkout releases/gcc-10
again, otherwise the worktree was in a detached state.  After that, everything
worked fine.

Thanks,
Harald

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

[Harald Anlauf]

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:

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)?

Thanks,
Harald

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

[Thomas Koenig]

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

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

[Harald Anlauf]

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

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

[Thomas Koenig]

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