[PR libfortran/62768] Handle filenames with embedded nulls

[PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

Hi,

when opening a file with a name containing an ASCII null character,
the name is truncated at the first null since the IO API's take
null-terminated C strings. By storing such a C string in gfc_unit
instead of the original Fortran string INQUIRE for the file name will
return the correct name, and the code is simplified slightly. It also
fixes an bug in open.c(already_open) for the !HAVE_UNLINK_OPEN_FILE
case in that the file is unlinked before freeing the name (which I
can't test but seems obvious).

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/62768
    * io/io.h (gfc_unit): Store C string for the filename.
    * io/close.c (st_close): Use gfc_unit.filename.
    * io/inquire.c (inquire_via_unit): Likewise.
    * io/open.c (new_unit): Likewise.
    (already_open): Likewise, unlink file before freeing filename.
    * io/unit.c (init_units): Likewise.
    (close_unit_1): Likewise.
    (filename_from_unit): Likewise.
    * io/unix.c (compare_file_filename): Likewise.
    (find_file0): Likewise.
    (delete_file): Likewise.


-- 
Janne Blomqvist

[-- Attachment #2: nullfile.diff --]
[-- Type: text/plain, Size: 6374 bytes --]

diff --git a/libgfortran/io/close.c b/libgfortran/io/close.c
index 55f49da..d0d49f6 100644
--- a/libgfortran/io/close.c
+++ b/libgfortran/io/close.c
@@ -72,7 +72,7 @@ st_close (st_parameter_close *clp)
 	    generate_error (&clp->common, LIBERROR_BAD_OPTION,
 			    "Can't KEEP a scratch file on CLOSE");
 #if !HAVE_UNLINK_OPEN_FILE
-	  path = fc_strdup (u->file, u->file_len);
+	  path = strdup (u->filename);
 #endif
 	}
       else
@@ -82,7 +82,7 @@ st_close (st_parameter_close *clp)
 #if HAVE_UNLINK_OPEN_FILE
 	      delete_file (u);
 #else
-	      path = fc_strdup (u->file, u->file_len);
+	      path = strdup (u->filename);
 #endif
             }
 	}
diff --git a/libgfortran/io/inquire.c b/libgfortran/io/inquire.c
index c41237c..4d03161 100644
--- a/libgfortran/io/inquire.c
+++ b/libgfortran/io/inquire.c
@@ -80,10 +80,10 @@ inquire_via_unit (st_parameter_inquire *iqp, gfc_unit * u)
 		memset (&iqp->name[tmplen], ' ', iqp->name_len - tmplen);
 	    }
 	  else /* If ttyname does not work, go with the default.  */
-	    fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
+	    cf_strcpy (iqp->name, iqp->name_len, u->filename);
 	}
       else
-	fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
+	cf_strcpy (iqp->name, iqp->name_len, u->filename);
 #elif defined __MINGW32__
       if (u->unit_number == options.stdin_unit)
 	fstrcpy (iqp->name, iqp->name_len, "CONIN$", sizeof("CONIN$"));
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 4e71d49..1e0d092 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -567,8 +567,9 @@ typedef struct gfc_unit
   array_loop_spec *ls;
   int rank;
 
-  int file_len;
-  char *file;
+  /* Name of the file at the time OPEN was executed, as a
+     null-terminated C string.  */
+  char *filename;
 
   /* The format hash table.  */
   struct format_hash_entry format_hash_table[FORMAT_HASH_SIZE];
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index b803859..67fa9ac 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -541,7 +541,6 @@ new_unit (st_parameter_open *opp, gfc_unit *u, unit_flags * flags)
 
   /* Create the unit structure.  */
 
-  u->file = xmalloc (opp->file_len);
   if (u->unit_number != opp->common.unit)
     internal_error (&opp->common, "Unit number changed");
   u->s = s;
@@ -618,8 +617,7 @@ new_unit (st_parameter_open *opp, gfc_unit *u, unit_flags * flags)
       u->strm_pos = stell (u->s) + 1;
     }
 
-  memmove (u->file, opp->file, opp->file_len);
-  u->file_len = opp->file_len;
+  u->filename = fc_strdup (opp->file, opp->file_len);
 
   /* Curiously, the standard requires that the
      position specifier be ignored for new files so a newly connected
@@ -685,20 +683,13 @@ already_open (st_parameter_open *opp, gfc_unit * u, unit_flags * flags)
 	}
 
       u->s = NULL;
-      free (u->file);
-      u->file = NULL;
-      u->file_len = 0;
-
+ 
 #if !HAVE_UNLINK_OPEN_FILE
-      char *path = NULL;
-      if (u->file && u->flags.status == STATUS_SCRATCH)
-	path = fc_strdup (u->file, u->file_len);
-      if (path != NULL)
-	{
-	  unlink (path);
-	  free (path);
-	}
+      if (u->filename && u->flags.status == STATUS_SCRATCH)
+	unlink (u->filename);
 #endif
+     free (u->filename);
+     u->filename = NULL;
 
       u = new_unit (opp, u, flags);
       if (u != NULL)
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 22b315a..5cc51b5 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -587,9 +587,7 @@ init_units (void)
       u->recl = options.default_recl;
       u->endfile = NO_ENDFILE;
 
-      u->file_len = strlen (stdin_name);
-      u->file = xmalloc (u->file_len);
-      memmove (u->file, stdin_name, u->file_len);
+      u->filename = strdup (stdin_name);
 
       fbuf_init (u, 0);
     
@@ -618,9 +616,7 @@ init_units (void)
       u->recl = options.default_recl;
       u->endfile = AT_ENDFILE;
     
-      u->file_len = strlen (stdout_name);
-      u->file = xmalloc (u->file_len);
-      memmove (u->file, stdout_name, u->file_len);
+      u->filename = strdup (stdout_name);
       
       fbuf_init (u, 0);
 
@@ -648,9 +644,7 @@ init_units (void)
       u->recl = options.default_recl;
       u->endfile = AT_ENDFILE;
 
-      u->file_len = strlen (stderr_name);
-      u->file = xmalloc (u->file_len);
-      memmove (u->file, stderr_name, u->file_len);
+      u->filename = strdup (stderr_name);
       
       fbuf_init (u, 256);  /* 256 bytes should be enough, probably not doing
                               any kind of exotic formatting to stderr.  */
@@ -689,9 +683,8 @@ close_unit_1 (gfc_unit *u, int locked)
 
   delete_unit (u);
 
-  free (u->file);
-  u->file = NULL;
-  u->file_len = 0;
+  free (u->filename);
+  u->filename = NULL;
 
   free_format_hash_table (u);  
   fbuf_destroy (u);
@@ -804,7 +797,7 @@ filename_from_unit (int n)
 
   /* Get the filename.  */
   if (u != NULL)
-    return fc_strdup (u->file, u->file_len);
+    return strdup (u->filename);
   else
     return (char *) NULL;
 }
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 9ad293b..d30c6e5 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -1525,11 +1525,7 @@ compare_file_filename (gfc_unit *u, const char *name, int len)
       goto done;
     }
 # endif
-
-  if (len != u->file_len)
-    ret = 0;
-  else
-    ret = (memcmp(path, u->file, len) == 0);
+  ret = (strcmp(path, u->filename) == 0);
 #endif
  done:
   free (path);
@@ -1541,8 +1537,8 @@ compare_file_filename (gfc_unit *u, const char *name, int len)
 # define FIND_FILE0_DECL struct stat *st
 # define FIND_FILE0_ARGS st
 #else
-# define FIND_FILE0_DECL uint64_t id, const char *file, gfc_charlen_type file_len
-# define FIND_FILE0_ARGS id, file, file_len
+# define FIND_FILE0_DECL uint64_t id, const char *path
+# define FIND_FILE0_ARGS id, path
 #endif
 
 /* find_file0()-- Recursive work function for find_file() */
@@ -1574,7 +1570,7 @@ find_file0 (gfc_unit *u, FIND_FILE0_DECL)
     }
   else
 # endif
-    if (compare_string (u->file_len, u->file, file_len, file) == 0)
+    if (strcmp (u->filename, path) == 0)
       return u;
 #endif
 
@@ -1718,10 +1714,7 @@ flush_all_units (void)
 int
 delete_file (gfc_unit * u)
 {
-  char *path = fc_strdup (u->file, u->file_len);
-  int err = unlink (path);
-  free (path);
-  return err;
+  return unlink (u->filename);
 }
 
 

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

Ping.

On Fri, Sep 5, 2014 at 3:54 AM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> Hi,
>
> when opening a file with a name containing an ASCII null character,
> the name is truncated at the first null since the IO API's take
> null-terminated C strings. By storing such a C string in gfc_unit
> instead of the original Fortran string INQUIRE for the file name will
> return the correct name, and the code is simplified slightly. It also
> fixes an bug in open.c(already_open) for the !HAVE_UNLINK_OPEN_FILE
> case in that the file is unlinked before freeing the name (which I
> can't test but seems obvious).
>
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>
> 2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
>
>     PR libfortran/62768
>     * io/io.h (gfc_unit): Store C string for the filename.
>     * io/close.c (st_close): Use gfc_unit.filename.
>     * io/inquire.c (inquire_via_unit): Likewise.
>     * io/open.c (new_unit): Likewise.
>     (already_open): Likewise, unlink file before freeing filename.
>     * io/unit.c (init_units): Likewise.
>     (close_unit_1): Likewise.
>     (filename_from_unit): Likewise.
>     * io/unix.c (compare_file_filename): Likewise.
>     (find_file0): Likewise.
>     (delete_file): Likewise.
>
>
> --
> Janne Blomqvist



-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[FX]

>> 2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
>> 
>>    PR libfortran/62768
>>    * io/io.h (gfc_unit): Store C string for the filename.
>>    * io/close.c (st_close): Use gfc_unit.filename.
>>    * io/inquire.c (inquire_via_unit): Likewise.
>>    * io/open.c (new_unit): Likewise.
>>    (already_open): Likewise, unlink file before freeing filename.
>>    * io/unit.c (init_units): Likewise.
>>    (close_unit_1): Likewise.
>>    (filename_from_unit): Likewise.
>>    * io/unix.c (compare_file_filename): Likewise.
>>    (find_file0): Likewise.
>>    (delete_file): Likewise.

OK, if you add a runtime testcase.

I tried to think of other characters we might want to sanitize/special case, but at least on Unix/POSIX only NUL and / are fundamentally different. It might make sense to think about it for Windows targets.

FX

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Tue, Sep 16, 2014 at 11:17 AM, FX <fxcoudert@gmail.com> wrote:
>>> 2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
>>>
>>>    PR libfortran/62768
>>>    * io/io.h (gfc_unit): Store C string for the filename.
>>>    * io/close.c (st_close): Use gfc_unit.filename.
>>>    * io/inquire.c (inquire_via_unit): Likewise.
>>>    * io/open.c (new_unit): Likewise.
>>>    (already_open): Likewise, unlink file before freeing filename.
>>>    * io/unit.c (init_units): Likewise.
>>>    (close_unit_1): Likewise.
>>>    (filename_from_unit): Likewise.
>>>    * io/unix.c (compare_file_filename): Likewise.
>>>    (find_file0): Likewise.
>>>    (delete_file): Likewise.
>
> OK, if you add a runtime testcase.

Thanks for the review, committed as r215307. I added the testcase
below as gfortran.dg/filename_null.f90:

! { dg-do run }
! PR 62768
! Filenames with embedded NULL characters are truncated, make sure
! inquire reports the correct truncated name.
program filename_null
  implicit none
  character(len=15), parameter :: s = "hello" // achar(0) // "world", &
       s2 = "hello"
  character(len=15) :: r
  logical :: l
  open(10, file=s)
  inquire(unit=10, name=r)
  if (r /= s2) call abort()
  inquire(file=s2, exist=l)
  if (.not. l) call abort()
  close(10, status="delete")
end program filename_null


> I tried to think of other characters we might want to sanitize/special case, but at least on Unix/POSIX only NUL and / are fundamentally different. It might make sense to think about it for Windows targets.

IIRC on Windows only printable UTF-16 characters are allowed, and like
POSIX NULL and / are special. Anyway, if one does something which is
not allowed, the OS api's should report an error. The bug I fixed was
fundamentally about OS api's expecting null-terminated C strings, so
the strings were truncated in the Fortran->C string conversion before
calling the OS API's.

-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2683 bytes --]

On Wed, 17 Sep 2014, Janne Blomqvist wrote:
> On Tue, Sep 16, 2014 at 11:17 AM, FX <fxcoudert@gmail.com> wrote:
> >>> 2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
> >>>
> >>>    PR libfortran/62768
> >>>    * io/io.h (gfc_unit): Store C string for the filename.
> >>>    * io/close.c (st_close): Use gfc_unit.filename.
> >>>    * io/inquire.c (inquire_via_unit): Likewise.
> >>>    * io/open.c (new_unit): Likewise.
> >>>    (already_open): Likewise, unlink file before freeing filename.
> >>>    * io/unit.c (init_units): Likewise.
> >>>    (close_unit_1): Likewise.
> >>>    (filename_from_unit): Likewise.
> >>>    * io/unix.c (compare_file_filename): Likewise.
> >>>    (find_file0): Likewise.
> >>>    (delete_file): Likewise.
> >
> > OK, if you add a runtime testcase.
>
> Thanks for the review, committed as r215307.

Something went wrong.  For cris-elf:

libtool: compile:  /tmp/hpautotest-gcc1/cris-elf/gccobj/./gcc/xgcc -B/tmp/hpautotest-gcc1/cris-elf/gccobj/./gcc/ -nostdinc -B/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/newlib/ -isystem /tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/newlib/targ-include -isystem /tmp/hpautotest-gcc1/gcc/newlib/libc/include -B/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libgloss/cris -L/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libgloss/libnosys -L/tmp/hpautotest-gcc1/gcc/libgloss/cris -B/tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/bin/ -B/tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/lib/ -isystem /tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/include -isystem /tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/sys-include -DHAVE_CONFIG_H -I. -I/tmp/hpautotest-gcc1/gcc/libgfortran -iquote/tmp/hpautotest-gcc1/gcc/libgfortran/io -I/tmp/hpautotest-gcc1/gcc/libgfortran/../gcc -I/tmp/hpautotest-gcc1/gcc/libgfortran/../gcc/config -I../.././gcc -I/tmp/hpautotest-gcc1/gcc/libgfortran/../libgcc -I../libgcc -std=gnu11 -Wall 
 -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wextra -Wwrite-strings -fcx-fortran-rules -ffunction-sections -fdata-sections -g -O2 -MT inquire.lo -MD -MP -MF .deps/inquire.Tpo -c /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c -o inquire.o
/tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c: In function 'inquire_via_unit':
/tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:41: error: 'gfc_unit' has no member named 'file'
     fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
                                         ^
/tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:50: error: 'gfc_unit' has no member named 'file_len'
     fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
                                                  ^
make[3]: *** [inquire.lo] Error 1

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Wed, Sep 17, 2014 at 2:22 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Wed, 17 Sep 2014, Janne Blomqvist wrote:
>> On Tue, Sep 16, 2014 at 11:17 AM, FX <fxcoudert@gmail.com> wrote:
>> >>> 2014-09-05  Janne Blomqvist  <jb@gcc.gnu.org>
>> >>>
>> >>>    PR libfortran/62768
>> >>>    * io/io.h (gfc_unit): Store C string for the filename.
>> >>>    * io/close.c (st_close): Use gfc_unit.filename.
>> >>>    * io/inquire.c (inquire_via_unit): Likewise.
>> >>>    * io/open.c (new_unit): Likewise.
>> >>>    (already_open): Likewise, unlink file before freeing filename.
>> >>>    * io/unit.c (init_units): Likewise.
>> >>>    (close_unit_1): Likewise.
>> >>>    (filename_from_unit): Likewise.
>> >>>    * io/unix.c (compare_file_filename): Likewise.
>> >>>    (find_file0): Likewise.
>> >>>    (delete_file): Likewise.
>> >
>> > OK, if you add a runtime testcase.
>>
>> Thanks for the review, committed as r215307.
>
> Something went wrong.  For cris-elf:
>
> libtool: compile:  /tmp/hpautotest-gcc1/cris-elf/gccobj/./gcc/xgcc -B/tmp/hpautotest-gcc1/cris-elf/gccobj/./gcc/ -nostdinc -B/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/newlib/ -isystem /tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/newlib/targ-include -isystem /tmp/hpautotest-gcc1/gcc/newlib/libc/include -B/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libgloss/cris -L/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libgloss/libnosys -L/tmp/hpautotest-gcc1/gcc/libgloss/cris -B/tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/bin/ -B/tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/lib/ -isystem /tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/include -isystem /tmp/hpautotest-gcc1/cris-elf/pre/cris-elf/sys-include -DHAVE_CONFIG_H -I. -I/tmp/hpautotest-gcc1/gcc/libgfortran -iquote/tmp/hpautotest-gcc1/gcc/libgfortran/io -I/tmp/hpautotest-gcc1/gcc/libgfortran/../gcc -I/tmp/hpautotest-gcc1/gcc/libgfortran/../gcc/config -I../.././gcc -I/tmp/hpautotest-gcc1/gcc/libgfortran/../libgcc -I../libgcc -std=gnu11 -Wall
>  -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wextra -Wwrite-strings -fcx-fortran-rules -ffunction-sections -fdata-sections -g -O2 -MT inquire.lo -MD -MP -MF .deps/inquire.Tpo -c /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c -o inquire.o
> /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c: In function 'inquire_via_unit':
> /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:41: error: 'gfc_unit' has no member named 'file'
>      fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>                                          ^
> /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:50: error: 'gfc_unit' has no member named 'file_len'
>      fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>                                                   ^
> make[3]: *** [inquire.lo] Error 1
>
> brgds, H-P

Oops, I forgot to update some parts in an #ifdef branch that isn't
taken on my target. I'll try to find time to fix it later tonight. If
you're in a hurry, just replace

fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);

with

cf_strcpy (iqp->name, iqp->name_len, u->filename);

in inquire.c.


-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Wed, 17 Sep 2014, Janne Blomqvist wrote:
> > /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:41: error: 'gfc_unit' has no member named 'file'
> >      fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> >                                          ^
> > /tmp/hpautotest-gcc1/gcc/libgfortran/io/inquire.c:97:50: error: 'gfc_unit' has no member named 'file_len'
> >      fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> >                                                   ^
> > make[3]: *** [inquire.lo] Error 1
> >
> > brgds, H-P
>
> Oops, I forgot to update some parts in an #ifdef branch that isn't
> taken on my target. I'll try to find time to fix it later tonight. If
> you're in a hurry, just replace
>
> fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>
> with
>
> cf_strcpy (iqp->name, iqp->name_len, u->filename);
>
> in inquire.c.

Thanks, build completes and I'll commit the following as obvious
if there are no regressions.  (The indentation change is
correct; lining up with the "else".  Note the closing brace.)

	* libgfortran/io/inquire.c (inquire_via_unit)
	[!HAVE_TTYNAME && !HAVE_TTYNAME_R && !__MINGW32__]: Adjust for
	last commit.

Index: libgfortran/io/inquire.c
===================================================================
--- libgfortran/io/inquire.c	(revision 215321)
+++ libgfortran/io/inquire.c	(working copy)
@@ -94,7 +94,7 @@ inquire_via_unit (st_parameter_inquire *
       else
 	fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
 #else
-    fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
+      cf_fstrcpy (iqp->name, iqp->name_len, u->filename);
 #endif
     }

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
> On Wed, 17 Sep 2014, Janne Blomqvist wrote:
> > Oops, I forgot to update some parts in an #ifdef branch that isn't
> > taken on my target. I'll try to find time to fix it later tonight. If
> > you're in a hurry, just replace
> >
> > fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> >
> > with
> >
> > cf_strcpy (iqp->name, iqp->name_len, u->filename);
> >
> > in inquire.c.
>
> Thanks, build completes and I'll commit the following as obvious
> if there are no regressions.

Since there are 25 related regressions, not committed.
There must be something else amiss too.

(Maybe you can find it on your platform by artificially
disabling HAVE_TTYNAME && HAVE_TTYNAME_R.)

+gfortran.sum gfortran.dg/f2003_inquire_1.f03
+gfortran.sum gfortran.dg/inquire.f90
+gfortran.sum gfortran.dg/inquire_13.f90
+gfortran.sum gfortran.dg/inquire_15.f90
+gfortran.sum gfortran.dg/inquire_16.f90
+gfortran.sum gfortran.dg/inquire_6.f90
+gfortran.sum gfortran.dg/inquire_7.f90
+gfortran.sum gfortran.dg/inquire_9.f90
+gfortran.sum gfortran.dg/inquire_size.f90
+gfortran.sum gfortran.dg/large_unit_1.f90
+gfortran.sum gfortran.dg/large_unit_2.f90
+gfortran.sum gfortran.dg/negative_unit.f
+gfortran.sum gfortran.dg/negative_unit_int8.f
+gfortran.sum gfortran.dg/open_negative_unit_1.f90
+gfortran.sum gfortran.dg/pr20950.f
+gfortran.sum gfortran.dg/streamio_10.f90
+gfortran.sum gfortran.dg/streamio_16.f90
+gfortran.sum gfortran.dg/streamio_3.f90
+gfortran.sum gfortran.dg/streamio_8.f90
+gfortran.sum gfortran.dg/unf_io_convert_4.f90
+gfortran.sum gfortran.fortran-torture/execute/inquire_1.f90
+gfortran.sum gfortran.fortran-torture/execute/inquire_2.f90
+gfortran.sum gfortran.fortran-torture/execute/inquire_3.f90
+gfortran.sum gfortran.fortran-torture/execute/inquire_4.f90
+gfortran.sum gfortran.fortran-torture/execute/inquire_5.f90

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
> On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
> > On Wed, 17 Sep 2014, Janne Blomqvist wrote:
> > > Oops, I forgot to update some parts in an #ifdef branch that isn't
> > > taken on my target. I'll try to find time to fix it later tonight. If
> > > you're in a hurry, just replace
> > >
> > > fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> > >
> > > with
> > >
> > > cf_strcpy (iqp->name, iqp->name_len, u->filename);
> > >
> > > in inquire.c.
> >
> > Thanks, build completes and I'll commit the following as obvious
> > if there are no regressions.
>
> Since there are 25 related regressions, not committed.
> There must be something else amiss too.

On the other hand, the tree *is* broken for some ports; I'd
prefer regressions to that.  So, unless you're onto this, how do
you feel about me committing the posted patch and opening a PR
for the regressions?

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Wed, Sep 17, 2014 at 11:36 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
>> On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
>> > On Wed, 17 Sep 2014, Janne Blomqvist wrote:
>> > > Oops, I forgot to update some parts in an #ifdef branch that isn't
>> > > taken on my target. I'll try to find time to fix it later tonight. If
>> > > you're in a hurry, just replace
>> > >
>> > > fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>> > >
>> > > with
>> > >
>> > > cf_strcpy (iqp->name, iqp->name_len, u->filename);
>> > >
>> > > in inquire.c.
>> >
>> > Thanks, build completes and I'll commit the following as obvious
>> > if there are no regressions.
>>
>> Since there are 25 related regressions, not committed.
>> There must be something else amiss too.
>
> On the other hand, the tree *is* broken for some ports; I'd
> prefer regressions to that.  So, unless you're onto this, how do
> you feel about me committing the posted patch and opening a PR
> for the regressions?

I committed

Index: inquire.c
===================================================================
--- inquire.c   (revision 215337)
+++ inquire.c   (working copy)
@@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire *
       else if (u->unit_number == options.stderr_unit)
        fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$"));
       else
-       fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
+       cf_strcpy (iqp->name, iqp->name_len, u->filename);
 #else
-    fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
+    cf_strcpy (iqp->name, iqp->name_len, u->filename);
 #endif
     }

as obvious (r215338).


-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Thu, 18 Sep 2014, Janne Blomqvist wrote:
> On Wed, Sep 17, 2014 at 11:36 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> > On the other hand, the tree *is* broken for some ports; I'd
> > prefer regressions to that.  So, unless you're onto this, how do
> > you feel about me committing the posted patch and opening a PR
> > for the regressions?
>
> I committed
>
> Index: inquire.c
> ===================================================================
> --- inquire.c   (revision 215337)
> +++ inquire.c   (working copy)
> @@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire *
>        else if (u->unit_number == options.stderr_unit)
>         fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$"));
>        else
> -       fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> +       cf_strcpy (iqp->name, iqp->name_len, u->filename);
>  #else
> -    fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
> +    cf_strcpy (iqp->name, iqp->name_len, u->filename);
>  #endif
>      }
>
> as obvious (r215338).

(Bah, without the indentation fixed...)

'k so we'll track the regressions in a PR.

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Thu, 18 Sep 2014, Janne Blomqvist wrote:
>> On Wed, Sep 17, 2014 at 11:36 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
>> > On the other hand, the tree *is* broken for some ports; I'd
>> > prefer regressions to that.  So, unless you're onto this, how do
>> > you feel about me committing the posted patch and opening a PR
>> > for the regressions?
>>
>> I committed
>>
>> Index: inquire.c
>> ===================================================================
>> --- inquire.c   (revision 215337)
>> +++ inquire.c   (working copy)
>> @@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire *
>>        else if (u->unit_number == options.stderr_unit)
>>         fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$"));
>>        else
>> -       fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>> +       cf_strcpy (iqp->name, iqp->name_len, u->filename);
>>  #else
>> -    fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len);
>> +    cf_strcpy (iqp->name, iqp->name_len, u->filename);
>>  #endif
>>      }
>>
>> as obvious (r215338).
>
> (Bah, without the indentation fixed...)

Fixed in r215340. :)

> 'k so we'll track the regressions in a PR.

Do you prefer to tack on to 62768 or a new PR?

Note that the r215338 fix only applies when using
INQUIRE(...NAME=...), so I don't think manually disabling
HAVE_TTYNAME{_R} helps in finding the cause of the regressions, while
I haven't gone through every testcase you mentioned none of the few
ones I did check inquired for the name. So there must be something
else. Can you check where it's actually failing? Is it failing the
testcase (call abort() ) or is there a segfault etc.? I suggest trying
e.g. gfortran.dg/inquire.f90 which is quite a simple testcase.



-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Thu, 18 Sep 2014, Janne Blomqvist wrote:
> On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> > 'k so we'll track the regressions in a PR.
>
> Do you prefer to tack on to 62768 or a new PR?

Hijacking 62768 for the purposes of reporting a regression for
its fix would not be proper.

> Note that the r215338 fix only applies when using
> INQUIRE(...NAME=...), so I don't think manually disabling
> HAVE_TTYNAME{_R} helps in finding the cause of the regressions, while
> I haven't gone through every testcase you mentioned none of the few
> ones I did check inquired for the name. So there must be something
> else. Can you check where it's actually failing? Is it failing the
> testcase (call abort() ) or is there a segfault etc.?

Will tell in a new PR, unless I see a really obvious fix.

> I suggest trying
> e.g. gfortran.dg/inquire.f90 which is quite a simple testcase.

Thanks.

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
> On Thu, 18 Sep 2014, Janne Blomqvist wrote:
> > On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> > > 'k so we'll track the regressions in a PR.
> >
> > Do you prefer to tack on to 62768 or a new PR?
>
> Hijacking 62768 for the purposes of reporting a regression for
> its fix would not be proper.

> Will tell in a new PR, unless I see a really obvious fix.

False alarm.  If you look back at the patch I posted, there's a
typo. :-}  Duly warned about, but I'd rather expect the build to
fail.

Apparently libgfortran is not compiled with -Werror, at least
not for crosses.  Maybe -Werror is there for native but I'm not
sure as I see some "warning: array subscript has type 'char'
[-Wchar-subscripts]" which seems generic and also some others.
Though no more than can be fixed or excepted, IMHO.

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Thu, Sep 18, 2014 at 11:14 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote:
>> On Thu, 18 Sep 2014, Janne Blomqvist wrote:
>> > On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
>> > > 'k so we'll track the regressions in a PR.
>> >
>> > Do you prefer to tack on to 62768 or a new PR?
>>
>> Hijacking 62768 for the purposes of reporting a regression for
>> its fix would not be proper.
>
>> Will tell in a new PR, unless I see a really obvious fix.
>
> False alarm.

Ok, good; I was a bit perplexed what could be wrong.

>  If you look back at the patch I posted, there's a
> typo. :-}  Duly warned about, but I'd rather expect the build to
> fail.

Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy,
and since we use std=gnu11 prototypes should be mandatory. Also, since
there's no symbol called cf_fstrcpy  so at least the linking should
fail. Unless the link picked up some old inquire.o file?

> Apparently libgfortran is not compiled with -Werror, at least
> not for crosses.  Maybe -Werror is there for native but I'm not
> sure as I see some "warning: array subscript has type 'char'
> [-Wchar-subscripts]" which seems generic and also some others.
> Though no more than can be fixed or excepted, IMHO.

No, Werror isn't used. It was tried, but apparently caused issues.
From the changelog:

2008-06-13  Tobias Burnus  <burnus@net-b.de>

        * configure.ac (AM_CFLAGS): Remove -Werror again.
        * configure: Regenerate.

2008-06-13  Tobias Burnus  <burnus@net-b.de>

        PR libgfortran/36518
        * configure.ac (AM_CFLAGS): Add -Werror.
        * configure: Regenerate.
        * m4/ifunction_logical.m4: Cast "n" to "(int)".
        * generated/any_l16.c: Regenerate.
        * generated/any_l2.c: Regenerate.
        * generated/all_l1.c: Regenerate.
        * generated/all_l2.c: Regenerate.
        * generated/all_l16.c: Regenerate.
        * generated/any_l4.c: Regenerate.
        * generated/count_4_l.c: Regenerate.
        * generated/count_8_l.c: Regenerate.
        * generated/all_l4.c: Regenerate.
        * generated/count_1_l.c: Regenerate.
        * generated/count_16_l.c: Regenerate.
        * generated/any_l8.c: Regenerate.
        * generated/count_2_l.c: Regenerate.
        * generated/any_l1.c: Regenerate.
        * generated/all_l8.c: Regenerate.


I have a vague recollection that there were issues with system headers
on non-glibc targets. It would be nice if Werror was used by default,
I think we've had a few cases where bugs have slipped past due to it.

-- 
Janne Blomqvist

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[N.M. Maclaren]

On Sep 18 2014, Janne Blomqvist wrote:
>
>> Apparently libgfortran is not compiled with -Werror, at least
>> not for crosses.  Maybe -Werror is there for native but I'm not
>> sure as I see some "warning: array subscript has type 'char'
>> [-Wchar-subscripts]" which seems generic and also some others.
>> Though no more than can be fixed or excepted, IMHO.
>
>No, Werror isn't used. It was tried, but apparently caused issues.
From the changelog:
>
>2008-06-13  Tobias Burnus  <burnus@net-b.de>
>
>        * configure.ac (AM_CFLAGS): Remove -Werror again.
>
>I have a vague recollection that there were issues with system headers
>on non-glibc targets. It would be nice if Werror was used by default,
>I think we've had a few cases where bugs have slipped past due to it.

I wasn't involved, but that sounds more than just likely!  I have had
that experience with several options, including Werror, pedantic and
specific standards ones.  My experience is that most vendors clean up
at least the standard C headers with time, and usually the more basic
POSIX ones, but any others often remain beyond redemption.

And what is not going to help is the ongoing incompatibilities
in de jure and de facto standards.  I have certainly seen standard
headers that would compile only with specific language selection
options.  Oh, yes, their COMPILER supported other ones - you just
couldn't use some important system headers with them :-(

If I get time, I will look at the libfortran header use and see if I
can make any useful specific comments.


Regards,
Nick Maclaren.

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Hans-Peter Nilsson]

On Thu, 18 Sep 2014, Janne Blomqvist wrote:
> >  If you look back at the patch I posted, there's a
> > typo. :-}  Duly warned about, but I'd rather expect the build to
> > fail.
>
> Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy,
> and since we use std=gnu11 prototypes should be mandatory. Also, since
> there's no symbol called cf_fstrcpy  so at least the linking should
> fail. Unless the link picked up some old inquire.o file?

For closure: no linking certainly *did* fail and no executable
was created for those tests; failing linking correctly counts as
a fail too.

> > Apparently libgfortran is not compiled with -Werror, at least
> > not for crosses.  Maybe -Werror is there for native but I'm not
> > sure as I see some "warning: array subscript has type 'char'
> > [-Wchar-subscripts]" which seems generic and also some others.
> > Though no more than can be fixed or excepted, IMHO.
>
> No, Werror isn't used. It was tried, but apparently caused issues.

'k.  Maybe -Werror=implicit-function-declaration is a middle
way.

brgds, H-P

Re: [PR libfortran/62768] Handle filenames with embedded nulls

[Janne Blomqvist]

On Thu, Sep 18, 2014 at 11:33 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Thu, 18 Sep 2014, Janne Blomqvist wrote:
>> >  If you look back at the patch I posted, there's a
>> > typo. :-}  Duly warned about, but I'd rather expect the build to
>> > fail.
>>
>> Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy,
>> and since we use std=gnu11 prototypes should be mandatory. Also, since
>> there's no symbol called cf_fstrcpy  so at least the linking should
>> fail. Unless the link picked up some old inquire.o file?
>
> For closure: no linking certainly *did* fail and no executable
> was created for those tests; failing linking correctly counts as
> a fail too.
>
>> > Apparently libgfortran is not compiled with -Werror, at least
>> > not for crosses.  Maybe -Werror is there for native but I'm not
>> > sure as I see some "warning: array subscript has type 'char'
>> > [-Wchar-subscripts]" which seems generic and also some others.
>> > Though no more than can be fixed or excepted, IMHO.
>>
>> No, Werror isn't used. It was tried, but apparently caused issues.
>
> 'k.  Maybe -Werror=implicit-function-declaration is a middle
> way.

Good idea. I committed r215741 as obvious which adds this to the compile flags.

I'm sure there are other warnings that can be enabled with -Werror=...
in a similar fashion, but this is a start at least. Another approach
would be to enable -Werror if some conditions are met. Such as

- native build
- --enable-maintainer-mode
- glibc target

I'm not in the mood to torture myself with autofoo to do this ATM, but
food for thought..

-- 
Janne Blomqvist