bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Ken Brown]

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

Cygwin supports the O_PATH flag starting with release 3.0.0, but it is buggy 
until release 3.0.8.  (Opening a FIFO with O_PATH causes a hang.)  The attached 
patches work around this problem.  The first patch (which still has a bug number 
that needs to be filled in) uses incorrect indentation for legibility.  The 
second patch fixes the indentation.

Ken

[-- Attachment #2: 0001-Avoid-O_PATH-on-versions-of-Cygwin-where-it-is-buggy.patch --]
[-- Type: text/plain, Size: 1611 bytes --]

From 8a350e90dc8d047e8372a53dd64fc178157b3f52 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 27 Jun 2019 14:54:09 -0400
Subject: [PATCH 1/2] Avoid O_PATH on versions of Cygwin where it is buggy

* src/dired.c [O_PATH] (use_o_path): New function.
(file_attributes): Use it.  (Bug#99999)
---
 src/dired.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/dired.c b/src/dired.c
index 493758292b..ac83a026de 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -36,6 +36,10 @@
 #include <filemode.h>
 #include <stat-time.h>
 
+#if defined CYGWIN && defined O_PATH
+#include <sys/utsname.h>
+#endif
+
 #include "lisp.h"
 #include "systime.h"
 #include "buffer.h"
@@ -921,6 +925,21 @@ DEFUN ("file-attributes", Ffile_attributes, Sfile_attributes, 1, 2, 0,
 			  id_format);
 }
 
+/* Cygwin supports O_PATH starting with release 3.0.0, but it is buggy
+   until release 3.0.8.  */
+#ifdef O_PATH
+static bool
+use_o_path (void)
+{
+# ifdef CYGWIN
+  struct utsname name;
+  return uname (&name) >= 0 && strverscmp (name.release, "3.0.8") >= 0;
+# else
+  return true;
+# endif
+}
+#endif
+
 static Lisp_Object
 file_attributes (int fd, char const *name,
 		 Lisp_Object dirname, Lisp_Object filename,
@@ -938,6 +957,8 @@ file_attributes (int fd, char const *name,
   int err = EINVAL;
 
 #ifdef O_PATH
+ if (use_o_path ())
+ {
   int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
   if (namefd < 0)
     err = errno;
@@ -960,6 +981,7 @@ file_attributes (int fd, char const *name,
 	  name = "";
 	}
     }
+ }
 #endif
 
   if (err == EINVAL)
-- 
2.21.0


[-- Attachment #3: 0002-src-dired.c-file_attributes-Fix-indentation.patch --]
[-- Type: text/plain, Size: 1693 bytes --]

From cd7bbc1e088033789b2948578fc85099ed11a2f1 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 27 Jun 2019 15:04:05 -0400
Subject: [PATCH 2/2] ; * src/dired.c (file_attributes): Fix indentation

---
 src/dired.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/dired.c b/src/dired.c
index ac83a026de..dfec2d89d8 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -958,30 +958,30 @@ file_attributes (int fd, char const *name,
 
 #ifdef O_PATH
  if (use_o_path ())
- {
-  int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
-  if (namefd < 0)
-    err = errno;
-  else
-    {
-      record_unwind_protect_int (close_file_unwind, namefd);
-      if (fstat (namefd, &s) != 0)
-	{
-	  err = errno;
-	  /* The Linux kernel before version 3.6 does not support
-	     fstat on O_PATH file descriptors.  Handle this error like
-	     missing support for O_PATH.  */
-	  if (err == EBADF)
-	    err = EINVAL;
-	}
-      else
-	{
-	  err = 0;
-	  fd = namefd;
-	  name = "";
-	}
-    }
- }
+   {
+     int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
+     if (namefd < 0)
+       err = errno;
+     else
+       {
+	 record_unwind_protect_int (close_file_unwind, namefd);
+	 if (fstat (namefd, &s) != 0)
+	   {
+	     err = errno;
+	     /* The Linux kernel before version 3.6 does not support
+		fstat on O_PATH file descriptors.  Handle this error like
+		missing support for O_PATH.  */
+	     if (err == EBADF)
+	       err = EINVAL;
+	   }
+	 else
+	   {
+	     err = 0;
+	     fd = namefd;
+	     name = "";
+	   }
+       }
+   }
 #endif
 
   if (err == EINVAL)
-- 
2.21.0

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Paul Eggert]

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

Thanks, but that patch intrudes on the mainline code a bit. It's simpler 
to just disable O_PATH on Cygwin until the old Cygwin versions die out. 
Also, there's a similar issue in fileio.c. I installed the attached, 
which should work around the problem in both source files.

[-- Attachment #2: 0001-Work-around-Cygwin-bug-with-O_PATH.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

From e7200b3cf9b4a6470197915a31c45739f078683b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 27 Jun 2019 13:05:05 -0700
Subject: [PATCH] Work around Cygwin bug with O_PATH

Problem reported by Ken Brown (Bug#36405).
* src/dired.c, src/fileio.c (O_PATH) [__CYGWIN__]: Undef.
---
 src/dired.c  | 4 ++++
 src/fileio.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/src/dired.c b/src/dired.c
index 493758292b..b8197d36a0 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -41,6 +41,10 @@
 #include "buffer.h"
 #include "coding.h"
 
+#ifdef __CYGWIN__
+# undef O_PATH /* Buggy in Cygwin 3.0.0 through 3.0.7.  */
+#endif
+
 #ifdef MSDOS
 #include "msdos.h"	/* for fstatat */
 #endif
diff --git a/src/fileio.c b/src/fileio.c
index ed1d2aedf3..e36118652c 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -61,6 +61,10 @@ Copyright (C) 1985-1988, 1993-2019 Free Software Foundation, Inc.
 # include <linux/fs.h>
 #endif
 
+#ifdef __CYGWIN__
+# undef O_PATH /* Buggy in Cygwin 3.0.0 through 3.0.7.  */
+#endif
+
 #ifdef WINDOWSNT
 #define NOMINMAX 1
 #include <windows.h>
-- 
2.21.0

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 27 Jun 2019 13:47:27 -0700
> Cc: 36405-done@debbugs.gnu.org
> 
> Thanks, but that patch intrudes on the mainline code a bit. It's simpler 
> to just disable O_PATH on Cygwin until the old Cygwin versions die out. 

IMO, that's too drastic.  Cygwin 3 is quite new, AFAIK, so it will be
years and years before the old versions die out; no one will remember
this bit by then.

What problems did you see/envision on other platforms with the
original patch?

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Ken Brown]

On 6/27/2019 4:47 PM, Paul Eggert wrote:
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -61,6 +61,10 @@ Copyright (C) 1985-1988, 1993-2019 Free Software Foundation, Inc.
>   # include <linux/fs.h>
>   #endif
>   
> +#ifdef __CYGWIN__
> +# undef O_PATH /* Buggy in Cygwin 3.0.0 through 3.0.7.  */
> +#endif

This hunk is not necessary.  The only use of O_PATH in fileio.c is coupled with 
O_DIRECTORY, and this is not affected by the bug.

But, in any case, I agree with Eli's point that your patch is too drastic.  I 
don't see how my patch is intrusive.

Ken

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Paul Eggert]

Ken Brown wrote:
> This hunk is not necessary.  The only use of O_PATH in fileio.c is coupled with
> O_DIRECTORY, and this is not affected by the bug.

Thanks, I reverted that part of the change. I'll also follow up to Eli's note.

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Paul Eggert]

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

Eli Zaretskii wrote:
> What problems did you see/envision on other platforms with the
> original patch?

Mainly the hassle of maintaining code with a lot of hard-to-read ifdefs in it. 
It's better if system-specific stuff is kept to a minimum in mainline code. 
Also, this O_PATH stuff is not that high of a priority, as the code works quite 
well in practice without O_PATH. (Also, if Cygwin botched O_PATH on FIFOs 
there's a reasonable chance it botched O_PATH elsewhere too. :-)

How about something like the attached patch instead? It's simpler and should be 
a bit faster on Cygwin than the original patch. I haven't tested it.

[-- Attachment #2: cygwin.diff --]
[-- Type: text/x-patch, Size: 1023 bytes --]

diff --git a/configure.ac b/configure.ac
index 8ff0e21fbf..a0e99ac393 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5734,6 +5734,9 @@ AC_DEFUN
     AC_MSG_WARN([[building Emacs on Cygwin 1.5 is not supported.]])
            echo
 	   ;;
+  cygwin,3.0.[0-8]'('*)
+    AC_DEFINE([HAVE_CYGWIN_O_PATH_BUG], 1,
+      [Define to 1 if opening a FIFO with O_PATH causes a hang.]);;
 esac
 
 # Remove any trailing slashes in these variables.
diff --git a/src/dired.c b/src/dired.c
index b8197d36a0..b700013f6a 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -41,10 +41,6 @@
 #include "buffer.h"
 #include "coding.h"
 
-#ifdef __CYGWIN__
-# undef O_PATH /* Buggy in Cygwin 3.0.0 through 3.0.7.  */
-#endif
-
 #ifdef MSDOS
 #include "msdos.h"	/* for fstatat */
 #endif
@@ -941,7 +937,7 @@ file_attributes (int fd, char const *name,
 
   int err = EINVAL;
 
-#ifdef O_PATH
+#if defined O_PATH && !defined HAVE_CYGWIN_O_PATH_BUG
   int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
   if (namefd < 0)
     err = errno;

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Eli Zaretskii]

> Cc: 36405@debbugs.gnu.org, kbrown@cornell.edu
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 28 Jun 2019 23:26:09 -0700
> 
> Eli Zaretskii wrote:
> > What problems did you see/envision on other platforms with the
> > original patch?
> 
> Mainly the hassle of maintaining code with a lot of hard-to-read ifdefs in it. 
> It's better if system-specific stuff is kept to a minimum in mainline code. 

I agree, of course.  But in this case we are talking about adding 2
#ifdef's, so the situation is not much worse than it was before.

> Also, this O_PATH stuff is not that high of a priority, as the code works quite 
> well in practice without O_PATH.

It was evidently important enough for us to use it on platforms that
support it.

> How about something like the attached patch instead? It's simpler and should be 
> a bit faster on Cygwin than the original patch. I haven't tested it.

Thanks, this is fine with me if it does the job.  Ken?

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Ken Brown]

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

On 6/29/2019 3:06 AM, Eli Zaretskii wrote:
>> Cc: 36405@debbugs.gnu.org, kbrown@cornell.edu
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Fri, 28 Jun 2019 23:26:09 -0700
>>
>> Eli Zaretskii wrote:
>>> What problems did you see/envision on other platforms with the
>>> original patch?
>>
>> Mainly the hassle of maintaining code with a lot of hard-to-read ifdefs in it.
>> It's better if system-specific stuff is kept to a minimum in mainline code.
> 
> I agree, of course.  But in this case we are talking about adding 2
> #ifdef's, so the situation is not much worse than it was before.
> 
>> Also, this O_PATH stuff is not that high of a priority, as the code works quite
>> well in practice without O_PATH.
> 
> It was evidently important enough for us to use it on platforms that
> support it.
> 
>> How about something like the attached patch instead? It's simpler and should be
>> a bit faster on Cygwin than the original patch. I haven't tested it.
> 
> Thanks, this is fine with me if it does the job.  Ken?

Fine with me too, after a small tweak.  Revised patch attached.

Thanks, Paul.

Ken

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

diff --git a/configure.ac b/configure.ac
index 8ff0e21fbf..774f8e5eb9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5734,6 +5734,9 @@ AC_DEFUN
     AC_MSG_WARN([[building Emacs on Cygwin 1.5 is not supported.]])
            echo
 	   ;;
+  cygwin,3.0.[[0-7]]'('*)
+    AC_DEFINE([HAVE_CYGWIN_O_PATH_BUG], 1,
+      [Define to 1 if opening a FIFO with O_PATH causes a hang.]);;
 esac
 
 # Remove any trailing slashes in these variables.
diff --git a/src/dired.c b/src/dired.c
index b8197d36a0..b700013f6a 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -41,10 +41,6 @@
 #include "buffer.h"
 #include "coding.h"
 
-#ifdef __CYGWIN__
-# undef O_PATH /* Buggy in Cygwin 3.0.0 through 3.0.7.  */
-#endif
-
 #ifdef MSDOS
 #include "msdos.h"	/* for fstatat */
 #endif
@@ -941,7 +937,7 @@ file_attributes (int fd, char const *name,
 
   int err = EINVAL;
 
-#ifdef O_PATH
+#if defined O_PATH && !defined HAVE_CYGWIN_O_PATH_BUG
   int namefd = openat (fd, name, O_PATH | O_CLOEXEC | O_NOFOLLOW);
   if (namefd < 0)
     err = errno;

bug#36405: 26.2.90; O_PATH problem on some versions of Cygwin

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> CC: "36405@debbugs.gnu.org" <36405@debbugs.gnu.org>
> Date: Sat, 29 Jun 2019 18:03:02 +0000
> 
> > Thanks, this is fine with me if it does the job.  Ken?
> 
> Fine with me too, after a small tweak.  Revised patch attached.

Go ahead and push, then.

Thanks.