Bug 83344 - Use of uninitialized memory with ASSOCIATE and strings
Summary: Use of uninitialized memory with ASSOCIATE and strings
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 6.3.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks: 78534
  Show dependency treegraph
 
Reported: 2017-12-09 15:35 UTC by Janne Blomqvist
Modified: 2018-02-28 17:36 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-12-09 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Janne Blomqvist 2017-12-09 15:35:32 UTC
Consider the testcase

subroutine foo()

  implicit none

  character(len=4) :: s
  s = "a"

  associate(w => trim(s))
  end associate

end subroutine foo

Compiling with "gfortran -c -fdump-tree-original" and looking at the 003t.original dump, one can see that the length of the character variable w varies randomly from one compilation to the next. Suggestion that it's reading uninitialized memory. valgrind doesn't find anything fishy, though, maybe due to extensive use of unions etc. in the frontend.

Per se, the 's = "a"' statement isn't necessary, it's just there to prevent trim(s) from being non-standards compliant.

Removing the trim() call, that is, associating w=>s directly makes this issue fo away. So the error is somehow in how the return value of trim(), simplified at compile time, is assigned to the length of w.

Tested with version 6.3 and trunk on x86_64, and trunk on powerpc64-unknown-linux-gnu.
Comment 1 Dominique d'Humieres 2017-12-09 15:49:53 UTC
Confirmed from 5.5.0 up to trunk (8.0). Compiling the test with 4.9.3 gives an error:

pr83344.f90:8.24:

  associate(w => trim(s))
                        1
Error: Entity with assumed character length at (1) must be a dummy argument or a PARAMETER
Comment 2 Janne Blomqvist 2017-12-11 22:06:05 UTC
Seems it's wrong at least to access value.character.length if the expression isn't a constant? So,

diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index d1a2368..017b9f7 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -8628,9 +8628,17 @@ resolve_assoc_var (gfc_symbol* sym, bool resolve_target)
        sym->ts.u.cl = target->ts.u.cl;
 
       if (!sym->ts.u.cl->length && !sym->ts.deferred)
-       sym->ts.u.cl->length
-         = gfc_get_int_expr (gfc_charlen_int_kind,
-                             NULL, target->value.character.length);
+       {
+         if (target->expr_type == EXPR_CONSTANT)
+           sym->ts.u.cl->length
+             = gfc_get_int_expr (gfc_charlen_int_kind,
+                                 NULL, target->value.character.length);
+         else
+           {
+             // What now?
+             // sym->ts.deferred = true; // Hack that doesn't quite work..
+           }
+       }
     }
 
   /* If the target is a good class object, so is the associate variable.  */
Comment 3 kargl 2017-12-12 18:34:49 UTC
(In reply to Janne Blomqvist from comment #2)
> Seems it's wrong at least to access value.character.length if the expression
> isn't a constant? So,
> 
> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
> index d1a2368..017b9f7 100644
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -8628,9 +8628,17 @@ resolve_assoc_var (gfc_symbol* sym, bool
> resolve_target)
>         sym->ts.u.cl = target->ts.u.cl;
>  
>        if (!sym->ts.u.cl->length && !sym->ts.deferred)
> -       sym->ts.u.cl->length
> -         = gfc_get_int_expr (gfc_charlen_int_kind,
> -                             NULL, target->value.character.length);
> +       {
> +         if (target->expr_type == EXPR_CONSTANT)
> +           sym->ts.u.cl->length
> +             = gfc_get_int_expr (gfc_charlen_int_kind,
> +                                 NULL, target->value.character.length);
> +         else
> +           {
> +             // What now?
> +             // sym->ts.deferred = true; // Hack that doesn't quite work..
> +           }
> +       }
>      }

Well, I went down the rabbit hole, and I have concluded that gfortran
simply handles a selector with CHARACTER type wrong.  The standard 
states

   The associating entity assumes the declared type and
   type parameters of the selector.

In the testcase of comment #1, this is CHARACTER(LEN=1).
Here's an even uglier testcase, 

program foo
   implicit none
   character(len=1) a
   character(len=2) b
   character(len=3) c
   a = 'a';    call bar(a)
   b = 'bb';   call bar(b)
   c = 'ccc';  call bar(c)
   a = 'a';    call bah(a)
   b = 'bb';   call bah(b)
   c = 'ccc';  call bah(c)
   contains
      subroutine bar(x)
         implicit none
         character(len=*), intent(in) :: x
         write(*,'(A,I0,A)') 'len('//trim(x)//') = ', len(x)
      end subroutine bar
      subroutine bah(x)
         implicit none
         character(len=*), intent(in) :: x
         write(*,'(A,I0,A)') 'len('//trim(x)//') = ', len(x)
         associate(y => x)
            write(*,'(A,I0,A)') 'len('//trim(y)//') = ', len(y)
         end associate
      end subroutine bah
end program foo

gfortran 6, 7, and trunk all give

% gfc6 -o z a.f90 && ./z
len(a) = 1
len(bb) = 2
len(ccc) = 3
len() = 0
len() = 0
len() = 0
len() = 0
len() = 0
len() = 0

In trans-stmt.c, gfc_trans_associate_var seems to be somewhat
complicated by the need to handle polymorphic entities, o I
get lost very quickly.
Comment 4 Steve Kargl 2017-12-12 18:49:54 UTC
On Tue, Dec 12, 2017 at 06:34:49PM +0000, kargl at gcc dot gnu.org wrote:
> 
> gfortran 6, 7, and trunk all give
> 
> % gfc6 -o z a.f90 && ./z
> len(a) = 1
> len(bb) = 2
> len(ccc) = 3
> len() = 0
> len() = 0
> len() = 0
> len() = 0
> len() = 0
> len() = 0
> 
> In trans-stmt.c, gfc_trans_associate_var seems to be somewhat
> complicated by the need to handle polymorphic entities, o I
> get lost very quickly.
> 

flang is the only other compiler I have.  It gives

% flang -o z a.f90 && ./z
len(a) = 1
len(bb) = 2
len(ccc) = 3
len(a) = 1
len() = 6975520
len(bb) = 2
len() = 6975488
len(ccc) = 3
len() = 6975520
Comment 5 Harald Anlauf 2017-12-12 20:00:32 UTC
(In reply to Steve Kargl from comment #4)
> flang is the only other compiler I have.  It gives
> 
> % flang -o z a.f90 && ./z
> len(a) = 1
> len(bb) = 2
> len(ccc) = 3
> len(a) = 1
> len() = 6975520
> len(bb) = 2
> len() = 6975488
> len(ccc) = 3
> len() = 6975520

Intel v15 gives the result you probably expected:

len(a) = 1
len(bb) = 2
len(ccc) = 3
len(a) = 1
len(a) = 1
len(bb) = 2
len(bb) = 2
len(ccc) = 3
len(ccc) = 3
Comment 6 Steve Kargl 2017-12-12 20:04:54 UTC
In resolve.c(resolve_assoc_var) one finds this chuck of code


  /* Fix up the type-spec for CHARACTER types.  */
  if (sym->ts.type == BT_CHARACTER && !sym->attr.select_type_temporary)
    {
      if (!sym->ts.u.cl)
	sym->ts.u.cl = target->ts.u.cl;

      if (!sym->ts.u.cl->length && !sym->ts.deferred)
	sym->ts.u.cl->length
	  = gfc_get_int_expr (gfc_default_integer_kind,
			      NULL, target->value.character.length);
    }

This code does handle an assumed length type parameter nor the
return from a CHARACTER function.
Comment 7 Steve Kargl 2017-12-12 20:05:47 UTC
On Tue, Dec 12, 2017 at 08:00:32PM +0000, anlauf at gmx dot de wrote:
> 
> Intel v15 gives the result you probably expected:
> 
> len(a) = 1
> len(bb) = 2
> len(ccc) = 3
> len(a) = 1
> len(a) = 1
> len(bb) = 2
> len(bb) = 2
> len(ccc) = 3
> len(ccc) = 3
> 

Yes, those are the results I expect.
Comment 8 Steve Kargl 2017-12-13 01:46:52 UTC
On Tue, Dec 12, 2017 at 08:04:54PM +0000, sgk at troutmask dot apl.washington.edu wrote:
> 
>   /* Fix up the type-spec for CHARACTER types.  */
>   if (sym->ts.type == BT_CHARACTER && !sym->attr.select_type_temporary)
>     {
>       if (!sym->ts.u.cl)
>         sym->ts.u.cl = target->ts.u.cl;
> 
>       if (!sym->ts.u.cl->length && !sym->ts.deferred)
>         sym->ts.u.cl->length
>           = gfc_get_int_expr (gfc_default_integer_kind,
>                               NULL, target->value.character.length);
>     }
> 

So, if I wrap the above code in #if 0 ... #endif.  I sort of
get what I expect.  For the reduce code,

program foo
   implicit none
   character(len=3) a
   a = 'abc';
   call bah(a)
   contains
      subroutine bah(x)
         implicit none
         character(len=*), intent(in) :: x
         print *, x, len(x)
         associate(y => x)
            print *, y, len(y)
         end associate
      end subroutine bah
end program foo

I get the dump-tree-original (with dt_param stuff removed) of

bah (character(kind=1)[1:_x] & restrict x, integer(kind=4) _x,
 character(kind=1)[1:_x] * y)
{
  {
    integer(kind=4) _x = integer(kind=4);
    character(kind=1)[1:_x] * y;

    {
      _gfortran_transfer_character_write (&dt_parm.0, x, _x);
      {
        integer(kind=4) D.3717;
        D.3717 = _x;
        _gfortran_transfer_integer_write (&dt_parm.0, &D.3717, 4);
      }
    }

    y = (character(kind=1)[1:_x] *) x;
    {
      _gfortran_transfer_character_write (&dt_parm.1, y, _x);
      {
        integer(kind=4) D.3721;
        D.3721 = _x;
        _gfortran_transfer_integer_write (&dt_parm.1, &D.3721, 4);
      }
    }
  }
}

It is not at all clear to me why y is passed as a hidden
argument to bah().  The dump looks reasonable with the use
of _x, the hidden length for the assumed length x. It 
also appears that y is treated as a pointer to x, but not
referenced that way in _gfortran_transfer_character_write.

However, I get

% gfcx -o z a.f90
during RTL pass: expand
a.f90:7:0:

       subroutine bah(x)
 
internal compiler error: in set_parm_default_def_partition, at tree-ssa-coalesce.c:1919
0x5faa24 set_parm_default_def_partition
        ../../gcc/gcc/tree-ssa-coalesce.c:1919
0xc15987 for_all_parms
        ../../gcc/gcc/tree-ssa-coalesce.c:1023
0xc16608 get_parm_default_def_partitions(_var_map*)
        ../../gcc/gcc/tree-ssa-coalesce.c:1936
0xbcde40 remove_ssa_form
        ../../gcc/gcc/tree-outof-ssa.c:971
0xbcde40 rewrite_out_of_ssa(ssaexpand*)
        ../../gcc/gcc/tree-outof-ssa.c:1174
0x829cc0 execute
        ../../gcc/gcc/cfgexpand.c:6180
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions
Comment 9 Janne Blomqvist 2017-12-14 21:14:06 UTC
With the patch in #2 (which in this case is equivalent to your patch in #8) I get on my charlen->size_t branch:

❯ gfortran -O0 -Wall -c a22.f90
a22.f90:20:0:

   associate(w4 => trim(s))
 
Warning: ‘.w4’ is used uninitialized in this function [-Wuninitialized]
a22.f90:20:0:

   associate(w4 => trim(s))
 
Error: size of variable ‘w4’ is too large


The warning is entirely correct, if one looks at the 003t.original dump one can see that .w4 is never assigned to:

foo ()
{
  character(kind=1) s[1:4];

  {
    integer(kind=8) .w4;
    character(kind=1) w4[1:.w4];

    {
      character(kind=1) * pstr.0;
      integer(kind=8) len.1;
      sizetype D.3727;
      sizetype D.3728;

      _gfortran_string_trim (&len.1, (void * *) &pstr.0, 4, &s);
      D.3727 = (sizetype) len.1;
      D.3728 = (sizetype) .w4;
      if (D.3728 != 0)
        {
          __builtin_memmove ((void *) &w4, (void *) pstr.0, (unsigned long) MIN_EXPR <NON_LVALUE_EXPR <D.3728>, NON_LVALUE_EXPR <D.3727>>);
          if (NON_LVALUE_EXPR <D.3727> < NON_LVALUE_EXPR <D.3728>)
            {
              __builtin_memset ((void *) &w4 + NON_LVALUE_EXPR <D.3727>, 32, (unsigned long) (NON_LVALUE_EXPR <D.3728> - NON_LVALUE_EXPR <D.3727>));
            }
        }
      if (len.1 > 0)
        {
          __builtin_free ((void *) pstr.0);
        }
    }
    L.1:;
  }
}

What the "w4 is too large" error message means I'm not sure. It might be that the uninitialized kind=8 .w4 value is large enough to trigger some kind of "this must be non-sensical" error in the middle-end.
Comment 10 Janne Blomqvist 2017-12-28 18:49:43 UTC
Author: jb
Date: Thu Dec 28 18:49:12 2017
New Revision: 256021

URL: https://gcc.gnu.org/viewcvs?rev=256021&root=gcc&view=rev
Log:
PR fortran/83344 Don't set bogus constant value

This patch does not fix PR 83344, but merely fixes an error where we
used to set a constant character length value from a non-constant
expression, and thus set it to some bogus value.

As a result of this, I have commented out part of the associate_22.f90
test which otherwise generates a warning message.

Regtested on x86_64-pc-linux-gnu.

gcc/fortran/ChangeLog:

2017-12-28  Janne Blomqvist  <jb@gcc.gnu.org>

	PR fortran/83344
	* resolve.c (resolve_assoc_var): Don't set the constant value
	unless the target is a constant expression.

gcc/testsuite/ChangeLog:

2017-12-28  Janne Blomqvist  <jb@gcc.gnu.org>

	PR fortran/83344
	* gfortran.dg/associate_22.f90: Comment out part of test.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/associate_22.f90
Comment 11 Janne Blomqvist 2018-02-01 19:47:47 UTC
Author: jb
Date: Thu Feb  1 19:47:15 2018
New Revision: 257310

URL: https://gcc.gnu.org/viewcvs?rev=257310&root=gcc&view=rev
Log:
PR 83975 Associate target with non-constant character length

When associating a variable of type character, if the length of the
target isn't known at compile time, generate an error. See PR 83344
for more details.

Regtested on x86_64-pc-linux-gnu.

gcc/fortran/ChangeLog:

2018-02-01  Janne Blomqvist  <jb@gcc.gnu.org>

	PR 83975
	PR 83344
	* resolve.c (resolve_assoc_var): Generate an error if
	target length unknown.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
Comment 12 Paul Thomas 2018-02-19 22:09:45 UTC
Author: pault
Date: Mon Feb 19 22:09:13 2018
New Revision: 257827

URL: https://gcc.gnu.org/viewcvs?rev=257827&root=gcc&view=rev
Log:
2018-02-19  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/83344
	PR fortran/83975
	* resolve.c (resolve_assoc_var): Rearrange the logic for the
	determination of the character length of associate names. If
	the associate name is missing a length expression or the length
	expression is not a constant and the target is not a variable,
	make the associate name allocatable and deferred length.
	* trans-decl.c (gfc_get_symbol_decl): Null the character length
	backend_decl for deferred length associate names that are not
	variables. Set 'length' to gfc_index_zero_node for character
	associate names, whose character length is a PARM_DECL.

2018-02-19  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/83344
	PR fortran/83975
	* gfortran.dg/associate_22.f90: Enable commented out test.
	* gfortran.dg/associate_36.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/associate_36.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/associate_22.f90
Comment 13 Janne Blomqvist 2018-02-23 12:01:02 UTC
AFAICT Paul Thomas nailed this in r257827, closing as fixed.
Comment 14 Paul Thomas 2018-02-28 17:36:51 UTC
Author: pault
Date: Wed Feb 28 17:36:20 2018
New Revision: 258076

URL: https://gcc.gnu.org/viewcvs?rev=258076&root=gcc&view=rev
Log:
2018-02-28  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/83901
	* trans-stmt.c (trans_associate_var): Make sure that the se
	expression is a pointer type before converting it to the symbol
	backend_decl type.

2018-02-28  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/83901
	* gfortran.dg/associate_37.f90: New test.

	PR fortran/83344
	* gfortran.dg/associate_36.f90: Add Steve Kargl as contributer.


Added:
    trunk/gcc/testsuite/gfortran.dg/associate_37.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-stmt.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/associate_36.f90