diff mbox series

[fortran] PR fortran/92142 - CFI_setpointer corrupts descriptor

Message ID e2aa95a2-2ab5-3998-4940-0bcf620e9358@gmail.com
State New
Headers show
Series [fortran] PR fortran/92142 - CFI_setpointer corrupts descriptor | expand

Commit Message

José Rui Faustino de Sousa Oct. 17, 2019, 4:44 p.m. UTC 
Hi all!

Proposed patch to solve the handling of the attribute value in the 
descriptor.

Patch tested only on x86_64-pc-linux-gnu.

CFI_setpointer does not check if it is setting a pointer and will set 
any type of object to the target.

CFI_setpointer will also change the pointer attribute of the pointer to 
whatever is the target's attribute corrupting the descriptor.

Thank you very much.

Best regards,
José Rui

2019-10-17  José Rui Faustino de Sousa  <jrfsousa@gmail.com>

  PR fortran/92142
  * ISO_Fortran_binding.c (CFI_setpointer): Add check to verify if the
  object being set (result) is really a pointer. Remove two instances
  where the result attribute value is overwritten.

2019-10-17  José Rui Faustino de Sousa  <jrfsousa@gmail.com>

  PR fortran/92142
  * ISO_Fortran_binding_15.f90: New test.
  * ISO_Fortran_binding_15.c: Additional source.

Comments

Tobias Burnus Oct. 17, 2019, 5:29 p.m. UTC | #1 
Hi,

+      fprintf (stderr, "CFI_setpointer: Result is NULL.\n");
…
> + return CFI_INVALID_DESCRIPTOR;
> +! { dg-do run }
> +! { dg-additional-options "-fbounds-check" }
> +! { dg-additional-sources ISO_Fortran_binding_15.c }


If you generate to stdout/stderr like in this case, I think it makes 
sense to also check for this output using "{dg-output …}".

Otherwise, it looks okay at a glance – but I defer the proper review to 
either someone else or to later.

Another question would be: Is it always guaranteed that 
result->attribute  is set? I am asking because it resembles to the 
untrained eye the code at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92027

And there, the result attribute is unset – that might be a bug in the C 
code of the test itself – or in libgomp. But it doesn't harm to quickly 
think about whether that can be an issue here as well or not.

Cheers,

Tobias
Paul Richard Thomas Oct. 18, 2019, 6:18 p.m. UTC | #2 
I will deal with this and various other issues associated with
ISO_Fortran_binding tomorrow.

Thanks for your help

Paul

On Thu, 17 Oct 2019 at 18:30, Tobias Burnus <burnus@net-b.de> wrote:
>
> Hi,
>
> +      fprintf (stderr, "CFI_setpointer: Result is NULL.\n");
> …
> > + return CFI_INVALID_DESCRIPTOR;
> > +! { dg-do run }
> > +! { dg-additional-options "-fbounds-check" }
> > +! { dg-additional-sources ISO_Fortran_binding_15.c }
>
>
> If you generate to stdout/stderr like in this case, I think it makes
> sense to also check for this output using "{dg-output …}".
>
> Otherwise, it looks okay at a glance – but I defer the proper review to
> either someone else or to later.
>
> Another question would be: Is it always guaranteed that
> result->attribute  is set? I am asking because it resembles to the
> untrained eye the code at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92027
>
> And there, the result attribute is unset – that might be a bug in the C
> code of the test itself – or in libgomp. But it doesn't harm to quickly
> think about whether that can be an issue here as well or not.
>
> Cheers,
>
> Tobias
>
José Rui Faustino de Sousa Oct. 29, 2019, 10:35 a.m. UTC | #3 
Hi,

On 17/10/19 17:29, Tobias Burnus wrote:
> If you generate to stdout/stderr like in this case, I think it makes 
> sense to also check for this output using "{dg-output …}".
> 

Added the suggested check and a few touches to comments and the error 
message.

> that might be a bug in the C 
> code of the test itself 
> 

I took a look and although there are problems with the code of the test 
I do not think they are relevant.

Index: gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c
===================================================================
--- gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c	(revision 277537)
+++ gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c	(working copy)
@@ -15,7 +15,7 @@
    bool err;
    CFI_CDESC_T(1) that;
    CFI_index_t lb[] = { 0, 0 };
-  CFI_index_t ub[] = { 4, 1 };
+  CFI_index_t ub[] = { 4, 0 };
    CFI_index_t st[] = { 2, 0 };
    int chksum[] = { 9, 36, 38 };

@@ -39,7 +39,7 @@

    if (*status != CFI_SUCCESS)
      {
-      printf("FAIL C: status is %i\n",status);
+      printf("FAIL C: status is %i\n", *status);
        return;
      }

Best regards,
José Rui
Tobias Burnus Nov. 11, 2019, 10:29 a.m. UTC | #4 
Hi José,

On 10/29/19 11:35 AM, José Rui Faustino de Sousa wrote:
> Added the suggested check and a few touches to comments and the error 
> message.
Thanks.
>> that might be a bug in the C code of the test itself
> I took a look and although there are problems with the code of the 
> test I do not think they are relevant.
>
> Index: gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c

I have included that patch.

Other remark: ChangeLog missing in the updated version of the patch. (I 
just saw that you did include it when posting initially.)

I have now committed the patch as Rev. 278048 to the trunk.
Thanks for the patch!

Cheers,

Tobias
Mark Eggleston Nov. 11, 2019, 1:58 p.m. UTC | #5 
Unfortunately ISO_Fortran_binding_16.f90 contains a typo resulting in:

FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -O0  (test for excess errors)
FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -O1  (test for excess errors)
FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -O2  (test for excess errors)
FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -O3 -fomit-frame-pointer 
-funroll-loops -fpeel-loops -ftracer -finline-functions  (test for 
excess errors)
FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -O3 -g (test for excess 
errors)
FAIL: gfortran.dg/ISO_Fortran_binding_16.f90   -Os  (test for excess errors)

the cause is that it refers to ISO_Fortran_binding_15.c instead of 
ISO_Fortran_binding_16.c in the dg-additional-sources directive.

regards,

Mark

On 11/11/2019 10:29, Tobias Burnus wrote:
> Hi José,
>
> On 10/29/19 11:35 AM, José Rui Faustino de Sousa wrote:
>> Added the suggested check and a few touches to comments and the error 
>> message.
> Thanks.
>>> that might be a bug in the C code of the test itself
>> I took a look and although there are problems with the code of the 
>> test I do not think they are relevant.
>>
>> Index: gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c
>
> I have included that patch.
>
> Other remark: ChangeLog missing in the updated version of the patch. 
> (I just saw that you did include it when posting initially.)
>
> I have now committed the patch as Rev. 278048 to the trunk.
> Thanks for the patch!
>
> Cheers,
>
> Tobias
>
Tobias Burnus Nov. 11, 2019, 3:57 p.m. UTC | #6 
On 11/11/19 2:58 PM, Mark Eggleston wrote:

> Unfortunately ISO_Fortran_binding_16.f90 contains a typo resulting in: 
> the cause is that it refers to ISO_Fortran_binding_15.c instead of 
> ISO_Fortran_binding_16.c in the dg-additional-sources directive.

I was sure that I tested it and committed the right version after the 
renaming was needed. (Paul's patch used the same file names).

In any case, it is now corrected in Rev. 278055.

Thanks,

Tobias
diff mbox series

Patch

Index: libgfortran/runtime/ISO_Fortran_binding.c
===================================================================
--- libgfortran/runtime/ISO_Fortran_binding.c	(revision 276937)
+++ libgfortran/runtime/ISO_Fortran_binding.c	(working copy)
@@ -795,13 +795,21 @@ 
  int CFI_setpointer (CFI_cdesc_t *result, CFI_cdesc_t *source,
  		    const CFI_index_t lower_bounds[])
  {
-  /* Result must not be NULL. */
-  if (unlikely (compile_options.bounds_check) && result == NULL)
+  /* Result must not be NULL and must be a Fortran pointer. */
+  if (unlikely (compile_options.bounds_check))
      {
-      fprintf (stderr, "CFI_setpointer: Result is NULL.\n");
-      return CFI_INVALID_DESCRIPTOR;
+      if (result == NULL)
+	{
+	  fprintf (stderr, "CFI_setpointer: Result is NULL.\n");
+	  return CFI_INVALID_DESCRIPTOR;
+	}
+
+      if (result->attribute != CFI_attribute_pointer)
+	{
+ 	  fprintf (stderr, "CFI_setpointer: Result is not a Fortran pointer.\n");
+ 	  return CFI_INVALID_ATTRIBUTE;
+ 	}
      }
-
    /* If source is NULL, the result is a C Descriptor that describes a
     * disassociated pointer. */
    if (source == NULL)
@@ -808,7 +816,6 @@ 
      {
        result->base_addr = NULL;
        result->version  = CFI_VERSION;
-      result->attribute = CFI_attribute_pointer;
      }
    else
      {
@@ -852,7 +859,6 @@ 

        /* Assign components to result. */
        result->version = source->version;
-      result->attribute = source->attribute;

        /* Dimension information. */
        for (int i = 0; i < source->rank; i++)
Index: gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.c
===================================================================
--- gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.c	(nonexistent)
+++ gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.c	(working copy)
@@ -0,0 +1,41 @@ 
+/* Test the fix of .  */
+
+/* #include "../../../libgfortran/ISO_Fortran_binding.h" */
+#include "ISO_Fortran_binding.h"
+
+#include <stdlib.h>
+
+int c_setpointer(CFI_cdesc_t *);
+
+int c_setpointer(CFI_cdesc_t *ip)
+{
+  CFI_cdesc_t *yp = NULL;
+  void *auxp = ip->base_addr;
+  int ierr;
+  int status;
+
+  /* Setting up the pointer */
+  ierr = 1;
+  yp = malloc(sizeof(*ip));
+  if (yp == NULL) return ierr;
+  status = CFI_establish(yp, NULL, CFI_attribute_pointer, ip->type, 
ip->elem_len, ip->rank, NULL);
+  if (status != CFI_SUCCESS) return ierr;
+  if (yp->attribute != CFI_attribute_pointer) return ierr;
+  /* Set the pointer to ip */
+  ierr = 2;
+  status = CFI_setpointer(yp, ip, NULL);
+  if (status != CFI_SUCCESS) return ierr;
+  if (yp->attribute != CFI_attribute_pointer) return ierr;
+  /* Set the pointer to NULL */
+  ierr = 3;
+  status = CFI_setpointer(yp, NULL, NULL);
+  if (status != CFI_SUCCESS) return ierr;
+  if (yp->attribute != CFI_attribute_pointer) return ierr;
+  /* "Set" the ip variable to yp (should not be possible) */
+  ierr = 4;
+  status = CFI_setpointer(ip, yp, NULL);
+  if (status != CFI_INVALID_ATTRIBUTE) return ierr;
+  if (ip->attribute != CFI_attribute_other) return ierr;
+  if (ip->base_addr != auxp) return ierr;
+  return 0;
+}
Index: gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.f90
===================================================================
--- gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.f90	(nonexistent)
+++ gcc/testsuite/gfortran.dg/ISO_Fortran_binding_15.f90	(working copy)
@@ -0,0 +1,22 @@ 
+! { dg-do run }
+! { dg-additional-options "-fbounds-check" }
+! { dg-additional-sources ISO_Fortran_binding_15.c }
+!
+!
+  use, intrinsic :: iso_c_binding, only: c_int
+
+  implicit none
+
+  interface
+    function c_setpointer(ip) result(ierr) bind(c)
+      use, intrinsic :: iso_c_binding, only: c_int
+      type(*), dimension(..), target :: ip
+      integer(c_int)                 :: ierr
+    end function c_setpointer
+  end interface
+
+  integer(c_int) :: it = 1
+
+  if (c_setpointer(it) /= 0) stop 1
+
+end