[Bug libfortran/66936] New: io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[Bug libfortran/66936] New: io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[keith.marshall at mailinator dot com]

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

            Bug ID: 66936
           Summary: io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the
                    basis that mkstemp() is available
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: keith.marshall at mailinator dot com
  Target Milestone: ---

Created attachment 36013
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36013&action=edit
Kludge to work around issue for mingw32

When building GCC, with FORTRAN language support, for mingw32 with the
mingwrt-3.21+ runtime library from MinGW.org installed, the build fails in
libgfortran/io/unix.c, because the tempfile_open() function gratuitously
assumes that, if HAVE_MKSTEMP is defined, then it must invoke:

  mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);

regardless of whether or not those access mode flags are defined.  Of course,
the assumption is invalid: while S_IXUSR may have some semblance of meaning on
a windows host, and is defined, S_IRWXG and S_IRWXO are meaningless, and are
not.

Now, I can kludge around this, with the attached patch, but I suspect that it
doesn't represent a true solution.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

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

--- Comment #1 from kargl at gcc dot gnu.org ---
(In reply to Keith Marshall from comment #0)
> Created attachment 36013 [details]
> Kludge to work around issue for mingw32
> 
> When building GCC, with FORTRAN language support, for mingw32 with the
> mingwrt-3.21+ runtime library from MinGW.org installed, the build fails in
> libgfortran/io/unix.c, because the tempfile_open() function gratuitously
> assumes that, if HAVE_MKSTEMP is defined, then it must invoke:
> 
>   mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);
> 
> regardless of whether or not those access mode flags are defined.  Of
> course, the assumption is invalid: while S_IXUSR may have some semblance of
> meaning on a windows host, and is defined, S_IRWXG and S_IRWXO are
> meaningless, and are not.
> 
> Now, I can kludge around this, with the attached patch, but I suspect that
> it doesn't represent a true solution.

The name of the language is Fortran.  The language has been called
Fortran since 1988 or so.

The correct fix would be to add test to configure to define
HAVE_S_IRWXG and HAVE_S_IRWXO.  A better fix would be for 
MingW32 to conform to POSIX.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[keith.marshall at mailinator dot com]

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

--- Comment #2 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to kargl from comment #1)
> The name of the language is Fortran.  The language has been called
> Fortran since 1988 or so.

It was always FORTRAN, in the days when I actually used the language, but how
you choose to capitalize it, or not, is completely irrelevant.

> The correct fix would be to add test to configure to define
> HAVE_S_IRWXG and HAVE_S_IRWXO.

Of course it would, but I'm not going to dirty my hands with your autoconf
code; this is your bug, and that is definitively your responsibility.

> A better fix would be for MingW32 to conform to POSIX.

No, it most definitely would not; that is never going to happen.  MinGW is
definitively aimed at Microsoft Windows compatibility.  Windows is not POSIX,
and nor is MinGW; nor will it ever be, nor will it even aspire to be.  S_IRWXG
and S_IRWXO have no place in the windows environment; it would be wrong for
MinGW to define them, (even if an appropriate pseudo-definition were ever
proposed), just as it is wrong for you to assume that availability of mkstemp()
implies that they should be defined.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P5
           Severity|normal                      |enhancement

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that mkstemp() is available

[sgk at troutmask dot apl.washington.edu]

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

--- Comment #3 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Sun, Jul 19, 2015 at 05:29:35PM +0000, keith.marshall at mailinator dot com
wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66936
> 
> --- Comment #2 from Keith Marshall <keith.marshall at mailinator dot com> ---
> (In reply to kargl from comment #1)
> > The name of the language is Fortran.  The language has been called
> > Fortran since 1988 or so.
> 
> It was always FORTRAN, in the days when I actually used the language, but how
> you choose to capitalize it, or not, is completely irrelevant.

Ignorance is bliss.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|io/unix.c gratuitously uses |io/unix.c gratuitously uses
                   |S_IRWXG and S_IRWXO on the  |S_IRWXG and S_IRWXO on the
                   |basis that mkstemp() is     |basis that umask() is
                   |available                   |available

--- Comment #4 from kargl at gcc dot gnu.org ---
Update title.  The code in question is

#ifdef HAVE_UMASK
  /* Temporarily set the umask such that the file has 0600 permissions.  */
  mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);
#endif

MinGW appears to define HAVE_UMASK, but MinGW seems to lack
a correctly written umask(3).

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[keith.marshall at mailinator dot com]

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

--- Comment #5 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to kargl from comment #4)
> Update title.  The code in question is
> 
> #ifdef HAVE_UMASK
>   /* Temporarily set the umask such that the file has 0600 permissions.  */
>   mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);
> #endif
> 
> MinGW appears to define HAVE_UMASK, but MinGW seems to lack
> a correctly written umask(3).

MinGW inherits its umask() implementation from Microsoft, in the shape of
whatever MSVCRT.DLL provides; (and yes, at one time Microsoft decided to rename
their umask() implementation to _umask(), but MinGW has always mapped those
renames back to their originals, for backwards compatibility). So, the bug is
that you gratuitously assume that any umask() implementation should conform to
POSIX, even when the platform is not POSIX. IF you aim to support non-POSIX
platforms, (as you do with the mingw32 target), you must be prepared to handle
non-POSIX implementations, such as this umask() --> _umask(), which is
documented at https://msdn.microsoft.com/en-us/library/5axxx3be.aspx

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[keith.marshall at mailinator dot com]

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

--- Comment #6 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to kargl from comment #4)
> Update title.  The code in question is
> 
> #ifdef HAVE_UMASK
>   /* Temporarily set the umask such that the file has 0600 permissions.  */
>   mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);
> #endif
> 
> MinGW appears to define HAVE_UMASK, but MinGW seems to lack
> a correctly written umask(3).

Just for the record: MinGW has had access to Microsoft's umask() implementation
for a very long time; HAVE_UMASK has always been defined, when compiling the
mingw32 builds of GCC.  However, this particular block of code is encapsulated
within an outer #ifdef HAVE_MKSTEMP block, and it is only recently that MinGW
has provided an implementation of mkstemp(), (Microsoft do not offer one),
which has caused HAVE_MKSTEMP to become defined, so exposing this (presumably
long-standing) bug.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

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

--- Comment #7 from kargl at gcc dot gnu.org ---
(In reply to Keith Marshall from comment #6)
> (In reply to kargl from comment #4)
> > Update title.  The code in question is
> > 
> > #ifdef HAVE_UMASK
> >   /* Temporarily set the umask such that the file has 0600 permissions.  */
> >   mode_mask = umask (S_IXUSR | S_IRWXG | S_IRWXO);
> > #endif
> > 
> > MinGW appears to define HAVE_UMASK, but MinGW seems to lack
> > a correctly written umask(3).
> 
> Just for the record: MinGW has had access to Microsoft's umask()
> implementation for a very long time; HAVE_UMASK has always been defined,
> when compiling the mingw32 builds of GCC.  However, this particular block of
> code is encapsulated within an outer #ifdef HAVE_MKSTEMP block, and it is
> only recently that MinGW has provided an implementation of mkstemp(),
> (Microsoft do not offer one), which has caused HAVE_MKSTEMP to become
> defined, so exposing this (presumably long-standing) bug.

So add

#define S_IRWXG 0
#define S_IRWXO 0

to the header file wherever mingw defines the available mask bits
for umask(3).  The bug is clearly in mingw were it gratuitously maps
umask() to _umask() without properly adding the mappings for the
mode_t argument bits of umask(3).

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[keith.marshall at mailinator dot com]

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

--- Comment #8 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to kargl from comment #7)
> So add
> 
> #define S_IRWXG 0
> #define S_IRWXO 0
> 
> to the header file wherever mingw defines the available mask bits
> for umask(3).  The bug is clearly in mingw were it gratuitously maps
> umask() to _umask() without properly adding the mappings for the
> mode_t argument bits of umask(3).

Absolutely not!  Those bits are utterly irrelevant for the windows (MinGW)
platform; to add them would be do nothing more than create confusion.  The mask
bits for umask(), on the windows platform are S_IREAD | S_IWRITE; those are the
only mask bits YOUR code should be passing to non-POSIX umask(5axxx3be.aspx).

This is NOT a MinGW bug; it's a GCC bug, and that's where it should be fixed. 
Until you do fix it, I have my work-around, (which I'm perfectly willing to
publish in MinGW forked source for GCC, prominently commented as a ghastly hack
to circumvent a gross upstream GCC bug).

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Keith Marshall from comment #8)
> (In reply to kargl from comment #7)
> > So add
> > 
> > #define S_IRWXG 0
> > #define S_IRWXO 0
> > 
> > to the header file wherever mingw defines the available mask bits
> > for umask(3).  The bug is clearly in mingw were it gratuitously maps
> > umask() to _umask() without properly adding the mappings for the
> > mode_t argument bits of umask(3).
> 
> Absolutely not!  Those bits are utterly irrelevant for the windows (MinGW)
> platform; to add them would be do nothing more than create confusion.  The
> mask bits for umask(), on the windows platform are S_IREAD | S_IWRITE; those
> are the only mask bits YOUR code should be passing to non-POSIX
> umask(5axxx3be.aspx).
> 
> This is NOT a MinGW bug; it's a GCC bug, and that's where it should be
> fixed.  Until you do fix it, I have my work-around, (which I'm perfectly
> willing to publish in MinGW forked source for GCC, prominently commented as
> a ghastly hack to circumvent a gross upstream GCC bug).


Well it is a libgfortran bug yes.  What we could do add to io/unix.c:
#if MINGW && !defined(S_IRWXG)
#define S_IRWXG 0
#endif
#if MINGW && !defined(S_IRWXO)
#define S_IRWXO 0
#endif

And that will allow it to work correctly.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[keith.marshall at mailinator dot com]

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

--- Comment #10 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to Andrew Pinski from comment #9)
> Well it is a libgfortran bug yes.

Which, being pedantic, makes it a GCC bug, because libgfortran is a component
of GCC.

>  What we could do add to io/unix.c:
> #if MINGW && !defined(S_IRWXG)
> #define S_IRWXG 0
> #endif
> #if MINGW && !defined(S_IRWXO)
> #define S_IRWXO 0
> #endif
> 
> And that will allow it to work correctly.

Conceptually, yes; you'd actually need to test on __MINGW32__, or maybe even
more generically on _WIN32, rather than on (nonexistent) MINGW.  However, that
seems every bit as much of a kludge as my own work-around.  Realistically,
calling umask() in this context on native windows would seem to be completely
redundant anyway; I can't see how it will actually achieve anything, since all
it will do will be to reproduce the already existing state, at the time of the
call.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-08-06
                 CC|                            |fxcoudert at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |fxcoudert at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #11 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
What I do not understand is how this didn't show up before now, since the
relevant libgfortran code was committed in 2012, and the mingw-w64 project has
been providing gfortran builds throughout this time.


Other than that, I would simply suggest we don't use umask() on mingw targets
(no need to make useless system calls):

Index: io/unix.c
===================================================================
--- io/unix.c   (revision 226632)
+++ io/unix.c   (working copy)
@@ -1083,6 +1083,14 @@ set_close_on_exec (int fd __attribute__ 
 }


+/* On mingw, we don't use umask in the function below, because it
+   doesn't support the user/group/other-based permissions.  */
+
+#ifdef __MINGW32__
+#undef HAVE_UMASK
+#endif
+
+
 /* Helper function for tempfile(). Tries to open a temporary file in
    the directory specified by tempdir. If successful, the file name is
    stored in fname and the descriptor returned. Returns -1 on


Can someone (Keith?) actually confirm that this restores the build on mingw32?
(It should, but double-checking never hurts.)

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[sgk at troutmask dot apl.washington.edu]

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

--- Comment #12 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Thu, Aug 06, 2015 at 04:57:15PM +0000, fxcoudert at gcc dot gnu.org wrote:
> 
> Index: io/unix.c
> ===================================================================
> --- io/unix.c   (revision 226632)
> +++ io/unix.c   (working copy)
> @@ -1083,6 +1083,14 @@ set_close_on_exec (int fd __attribute__ 
>  }
> 
> 
> +/* On mingw, we don't use umask in the function below, because it
> +   doesn't support the user/group/other-based permissions.  */
> +
> +#ifdef __MINGW32__
> +#undef HAVE_UMASK
> +#endif
> +

Aren't there enough occurences of __MINGW32__ in io/unix.c?
Why add yet another kludge?  Put the #undef at line 48.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

--- Comment #13 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
(In reply to Steve Kargl from comment #12)
> Aren't there enough occurences of __MINGW32__ in io/unix.c?
> Why add yet another kludge?  Put the #undef at line 48.

Indeed, that works equally:

Index: io/unix.c
===================================================================
--- io/unix.c   (revision 226632)
+++ io/unix.c   (working copy)
@@ -101,6 +101,12 @@ id_from_fd (const int fd)
 }

 #endif /* HAVE_WORKING_STAT */
+
+
+/* On mingw, we don't use umask in tempfile_open(), because it
+   doesn't support the user/group/other-based permissions.  */
+#undef HAVE_UMASK
+
 #endif /* __MINGW32__ */

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

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

--- Comment #14 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Kai, as mingw maintainer, can you comment? Why did this issue appear recently?
And does the patch seem OK to you?

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[sgk at troutmask dot apl.washington.edu]

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

--- Comment #15 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Sun, Aug 09, 2015 at 08:53:36AM +0000, fxcoudert at gcc dot gnu.org wrote:
> Kai, as mingw maintainer, can you comment? Why did this issue appear recently?
> And does the patch seem OK to you?
> 

See comment #6.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

--- Comment #16 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Author: fxcoudert
Date: Sun Aug  9 16:43:42 2015
New Revision: 226750

URL: https://gcc.gnu.org/viewcvs?rev=226750&root=gcc&view=rev
Log:
        PR libfortran/66936
        * io/unix.c (__MINGW32__): Undefine HAVE_UMASK.

Modified:
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/io/unix.c

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

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

--- Comment #17 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
(In reply to Steve Kargl from comment #15)
> See comment #6.

Thanks Steve, I had missed that.

Given the history and reasons, I've committed the patch to restore build on
mingw32. Marking as fixed on trunk.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[keith.marshall at mailinator dot com]

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

--- Comment #18 from Keith Marshall <keith.marshall at mailinator dot com> ---
(In reply to Francois-Xavier Coudert from comment #17)
> Given the history and reasons, I've committed the patch to restore build on
> mingw32. Marking as fixed on trunk.

Thanks.  Looks like a cleaner way to achieve the same effect as my own Q&D
hack.  Reverted mine, and applied this to my 4.9.3 tree; can confirm that it
now builds successfully for mingw32, (as crossed-native build from GNU/Linux;
still can't build 5.x, but that's an entirely different issue).

I haven't written any Fortran code in near 15 years, and no longer run Windows
myself anyway, so haven't tested usability; let's push it out there, and see if
anyone in the MinGW.org community can identify any consequent problems.

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

--- Comment #20 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Author: fxcoudert
Date: Tue Aug 18 19:58:15 2015
New Revision: 226989

URL: https://gcc.gnu.org/viewcvs?rev=226989&root=gcc&view=rev
Log:
        PR libfortran/66936
        * io/unix.c (__MINGW32__): Undefine HAVE_UMASK.

Modified:
    branches/gcc-4_9-branch/libgfortran/ChangeLog
    branches/gcc-4_9-branch/libgfortran/io/unix.c

[Bug libfortran/66936] io/unix.c gratuitously uses S_IRWXG and S_IRWXO on the basis that umask() is available

[fxcoudert at gcc dot gnu.org]

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

--- Comment #21 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Now backported to all active branches.