[Bug libfortran/62188] New: Array bounds overrun in bessel_yn_r4/8/16 and other functions

[Bug libfortran/62188] New: Array bounds overrun in bessel_yn_r4/8/16 and other functions

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

            Bug ID: 62188
           Summary: Array bounds overrun in bessel_yn_r4/8/16 and other
                    functions
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vogt at linux dot vnet.ibm.com

There's an array bounds overrun in
gfortran/generated/bessel_r4.c:bessel_yn_r4().  The function is passed the
arguments n1 and n2 (n1 <= n2) and allocates memory for (n2 - n1 + 1) result
values:

  size_t size = n2 < n1 ? 0 : n2-n1+1;
  ...
  ret->base_addr = xmallocarray (size, sizeof (GFC_REAL_4));

But later on it writes from index 0 to n1 + n2:

  for (...; i <= n1+n2; ...)
    ...          ^^^^^
    ret->base_addr[i*stride] = ...;

The loop should be

  for (i = 2; i < n2-n1; i++)

instead.  The same bug exists in bessel_r8.c and bessel_r16.c and has been
present since at least gcc-4.8.  The existing test cases (bessel_<n>.f90) all
use 0 or low values > 0, so they have not caught this bug yet.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-08-19
                 CC|                            |kargl at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from kargl at gcc dot gnu.org ---
Confirmed.  I assume you found this by using a libc with
a malloc that has buffer overflow detection.  The obvious
patch is 

Index: m4/bessel.m4
===================================================================
--- m4/bessel.m4        (revision 213593)
+++ m4/bessel.m4        (working copy)
@@ -163,7 +163,7 @@ bessel_yn_r'rtype_kind` ('rtype` * const

   x2rev = GFC_REAL_'rtype_kind`_LITERAL(2.)/x;

-  for (i = 2; i <= n1+n2; i++)
+  for (i = 2; i <= n2 - n1; i++)
     {
 #if defined('rtype_name`_INFINITY)
       if (unlikely (last2 == -'rtype_name`_INFINITY))

I'll commit this later.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #3 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
(In reply to kargl from comment #1)
> I assume you found this by using a libc with
> a malloc that has buffer overflow detection.

Actually, no.  We inspected the function manually looking for the
cause of a test FAIL of bessel_7.f90 and just stumbled across it.

> I'll commit this later.

Great.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #4 from kargl at gcc dot gnu.org ---
Author: kargl
Date: Wed Aug 20 16:18:27 2014
New Revision: 214229

URL: https://gcc.gnu.org/viewcvs?rev=214229&root=gcc&view=rev
Log:
2014-08-20  Steven G. Kargl  <kargl@gcc.gnu.org>

    PR libgfortran/62188
    * m4/bessel.m4: Avoid indexing off the end of an array.
    * generated/bessel_r10.c: Regenerated.
    * generated/bessel_r16.c: Ditto.
    * generated/bessel_r4.c: Ditto.
    * generated/bessel_r8.c: Ditto.


Modified:
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/generated/bessel_r10.c
    trunk/libgfortran/generated/bessel_r16.c
    trunk/libgfortran/generated/bessel_r4.c
    trunk/libgfortran/generated/bessel_r8.c
    trunk/libgfortran/m4/bessel.m4

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #5 from kargl at gcc dot gnu.org ---
Author: kargl
Date: Wed Aug 20 16:22:20 2014
New Revision: 214230

URL: https://gcc.gnu.org/viewcvs?rev=214230&root=gcc&view=rev
Log:
2014-08-20  Steven G. Kargl  <kargl@gcc.gnu.org>

    PR libgfortran/62188
    * m4/bessel.m4: Avoid indexing off the end of an array.
    * generated/bessel_r10.c: Regenerated.
    * generated/bessel_r16.c: Ditto.
    * generated/bessel_r4.c: Ditto.
    * generated/bessel_r8.c: Ditto.

Modified:
    branches/gcc-4_9-branch/libgfortran/ChangeLog
    branches/gcc-4_9-branch/libgfortran/generated/bessel_r10.c
    branches/gcc-4_9-branch/libgfortran/generated/bessel_r16.c
    branches/gcc-4_9-branch/libgfortran/generated/bessel_r4.c
    branches/gcc-4_9-branch/libgfortran/generated/bessel_r8.c
    branches/gcc-4_9-branch/libgfortran/m4/bessel.m4

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #6 from kargl at gcc dot gnu.org ---
Author: kargl
Date: Wed Aug 20 16:23:55 2014
New Revision: 214231

URL: https://gcc.gnu.org/viewcvs?rev=214231&root=gcc&view=rev
Log:
2014-08-20  Steven G. Kargl  <kargl@gcc.gnu.org>

    PR libgfortran/62188
    * m4/bessel.m4: Avoid indexing off the end of an array.
    * generated/bessel_r10.c: Regenerated.
    * generated/bessel_r16.c: Ditto.
    * generated/bessel_r4.c: Ditto.
    * generated/bessel_r8.c: Ditto.

Modified:
    branches/gcc-4_8-branch/libgfortran/ChangeLog
    branches/gcc-4_8-branch/libgfortran/generated/bessel_r10.c
    branches/gcc-4_8-branch/libgfortran/generated/bessel_r16.c
    branches/gcc-4_8-branch/libgfortran/generated/bessel_r4.c
    branches/gcc-4_8-branch/libgfortran/generated/bessel_r8.c
    branches/gcc-4_8-branch/libgfortran/m4/bessel.m4

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from kargl at gcc dot gnu.org ---
(In reply to Dominik Vogt from comment #3)
> (In reply to kargl from comment #1)
> > I assume you found this by using a libc with
> > a malloc that has buffer overflow detection.
> 
> Actually, no.  We inspected the function manually looking for the
> cause of a test FAIL of bessel_7.f90 and just stumbled across it.

Ah, bessel_7.f90.  This test has some tolerances that are at the
very edge of numerical accuracy.  If your underlying libm implementation
of yn is poor, you'll get FAIL[ures].

> > I'll commit this later.
> 
> Great.

Committed to trunk and all open branches.

Thanks for the bug report.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to kargl from comment #7)
> > Actually, no.  We inspected the function manually looking for the
> > cause of a test FAIL of bessel_7.f90 and just stumbled across it.

Which would be:
  https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02311.html

> Committed to trunk and all open branches.

Steve, should we also add a test case for the "n1 < 0"?

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #9 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, Aug 26, 2014 at 01:08:22PM +0000, burnus at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188
> 
> Tobias Burnus <burnus at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |burnus at gcc dot gnu.org
> 
> --- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> ---
> (In reply to kargl from comment #7)
> > > Actually, no.  We inspected the function manually looking for the
> > > cause of a test FAIL of bessel_7.f90 and just stumbled across it.
> 
> Which would be:
>   https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02311.html
> 
> > Committed to trunk and all open branches.
> 
> Steve, should we also add a test case for the "n1 < 0"?
> 

I only looked at the specific issue raised by OP.  If 
calling bessel_yn() with n1 < 0 violates requirements 
of the standard, then yes we should probably check for
that situation.  I'll cast an eye over this later today.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #10 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, Aug 26, 2014 at 07:51:45AM -0700, Steve Kargl wrote:
> On Tue, Aug 26, 2014 at 01:08:22PM +0000, burnus at gcc dot gnu.org wrote:
> > 
> > Steve, should we also add a test case for the "n1 < 0"?
> > 
> 

Checking in general looks broken for bessel_yn and probably _jn.

program neumann_test
   implicit none
   integer n1, n2
   real x, b(10)
   x = 42.
   b = bessel_yn(-5, x)
   n1 = -5
   n2 = 5
   b = bessel_yn(n1, n2, x)
!  b = bessel_yn(-5, n2, x)
end program neumann_test

troutmask:sgk[223] gfc5x -o z bes.f90
troutmask:sgk[224] gfc5x -o z -std=f2008 bes.f90
bes.f90:8.17:

   b = bessel_yn(-5, x)
                 1
Error: GNU Extension: Negative argument N at (1)

First, this GNU Extension should not exists as bessel_[jy]n are
new in F2008 and I think we should adhere to the standard.

Second, umcommenting he last line in the program 
yields

bes.f90:12.7:

   b = bessel_yn(-5, n2, x)
       1
Error: Too many arguments in call to 'bessel_yn' at (1)

So, it appears the wrong checking function is getting called.
It may take me a day or 2 to unravel the issue.

[Bug libfortran/62188] Array bounds overrun in bessel_yn_r4/8/16 and other functions

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62188

--- Comment #11 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, Aug 26, 2014 at 10:53:58AM -0700, Steve Kargl wrote:
> On Tue, Aug 26, 2014 at 07:51:45AM -0700, Steve Kargl wrote:
> > On Tue, Aug 26, 2014 at 01:08:22PM +0000, burnus at gcc dot gnu.org wrote:
> > > 
> > > Steve, should we also add a test case for the "n1 < 0"?
> > > 
> > 
> 
> Checking in general looks broken for bessel_yn and probably _jn.
> 

I'm going to have to re-learn the internals of the intrinsics stuff.
>From intrinsic.c, we have

  add_sym_2 ("besjn", GFC_ISYM_JN, CLASS_ELEMENTAL, ACTUAL_NO, BT_REAL, dr,
         GFC_STD_GNU,
         gfc_check_besn, gfc_simplify_bessel_jn, gfc_resolve_besn,
         n, BT_INTEGER, di, REQUIRED, x, BT_REAL, dr, REQUIRED);

  make_alias ("bessel_jn", GFC_STD_F2008);

  add_sym_2 ("dbesjn", GFC_ISYM_JN, CLASS_ELEMENTAL, ACTUAL_NO, BT_REAL, dd,
         GFC_STD_GNU,
         gfc_check_besn, gfc_simplify_bessel_jn, gfc_resolve_besn,
         n, BT_INTEGER, di, REQUIRED, x, BT_REAL, dd, REQUIRED);

  add_sym_3 ("bessel_jn", GFC_ISYM_JN2, CLASS_TRANSFORMATIONAL, ACTUAL_NO,
         BT_REAL, dr, GFC_STD_F2008,
         gfc_check_bessel_n2, gfc_simplify_bessel_jn2,
         gfc_resolve_bessel_n2,
         "n1", BT_INTEGER, di, REQUIRED,"n2", BT_INTEGER, di, REQUIRED,
         x, BT_REAL, dr, REQUIRED);
  set_attr_value (3, true, true, true);

  make_generic ("bessel_jn", GFC_ISYM_JN, GFC_STD_F2008);

I don't see how bessel_jn can be made an alias to besjn (an entity with
2 args), and then a few lines later it is defined with 3 args and made
generic?  I think besjn and bessel_jn need to be dealt with separately
with "n2" in bessel_jn set as OPTIONAL.