Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Bruno Haible]

Paul Eggert wrote:
> we can use O_ACCMODE here.

Your patch is obviously better than my previous one. Thanks.

Bruno

Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Paul Eggert]

On 3/4/20 9:24 AM, Dan Gohman wrote:

Would
if (flags & ((O_CREAT | O_WRONLY | O_RDWR) & ~O_RDONLY))
be correct?


That would fix the problem for systems that define O_RDONLY as Hurd does,
while not breaking any known systems.

It wouldn't be correct on a hypothetical system


Thanks for mentioning this. We might as well fix the code for the hypothetical 
systems while we're at it, since we can use O_ACCMODE here. The issue also 
occurs in the 'open' wrapper. I installed the attached patch.
>From 1d86584739a712597a6fe3a62ec412238f0f13f8 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 7 Mar 2020 11:02:05 -0800
Subject: [PATCH] open, openat: port to (O_RDWR | O_RDONLY) != 0
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Potential portability problem reported by Dan Gohman in:
https://lists.gnu.org/r/bug-gnulib/2020-03/msg0.html
* lib/open.c (open):
* lib/openat.c (rpl_openat):
Don’t assume O_RDONLY is disjoint from O_RDWR.
---
 ChangeLog| 9 +
 lib/open.c   | 4 +++-
 lib/openat.c | 4 +++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0eddfee99..f6bd5180c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-03-07  Paul Eggert  
+
+	open, openat: port to (O_RDWR | O_RDONLY) != 0
+	Potential portability problem reported by Dan Gohman in:
+	https://lists.gnu.org/r/bug-gnulib/2020-03/msg0.html
+	* lib/open.c (open):
+	* lib/openat.c (rpl_openat):
+	Don’t assume O_RDONLY is disjoint from O_RDWR.
+
 2020-03-07  Bruno Haible  
 
 	findprog, relocatable-prog: Ignore directories during PATH search.
diff --git a/lib/open.c b/lib/open.c
index 487194f66..bb180fde2 100644
--- a/lib/open.c
+++ b/lib/open.c
@@ -110,7 +110,9 @@ open (const char *filename, int flags, ...)
  directories,
- if O_WRONLY or O_RDWR is specified, open() must fail because the
  file does not contain a '.' directory.  */
-  if (flags & (O_CREAT | O_WRONLY | O_RDWR))
+  if ((flags & O_CREAT)
+  || (flags & O_ACCMODE) == O_RDWR
+  || (flags & O_ACCMODE) == O_WRONLY)
 {
   size_t len = strlen (filename);
   if (len > 0 && filename[len - 1] == '/')
diff --git a/lib/openat.c b/lib/openat.c
index 7d67dc4ca..fdbe83f43 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -100,7 +100,9 @@ rpl_openat (int dfd, char const *filename, int flags, ...)
  directories,
- if O_WRONLY or O_RDWR is specified, open() must fail because the
  file does not contain a '.' directory.  */
-  if (flags & (O_CREAT | O_WRONLY | (O_RDWR & ~O_RDONLY)))
+  if ((flags & O_CREAT)
+  || (flags & O_ACCMODE) == O_RDWR
+  || (flags & O_ACCMODE) == O_WRONLY)
 {
   size_t len = strlen (filename);
   if (len > 0 && filename[len - 1] == '/')
-- 
2.17.1

Re: patch, have find_in_given_path return file only

[Bruno Haible]

Hi,

Dmitry Goncharov wrote:
> A user reporter an issue in gnu make.
> https://savannah.gnu.org/bugs/index.php?57962.
> The issue is that find_in_given_path returns a directory which matches
> the desired name.

Indeed, while POSIX
  
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08
only says
  "The list shall be searched from beginning to end, applying the filename
   to each prefix, until an executable file with the specified name and
   appropriate execution permissions is found."
all shells that I've tested continue searching when they've hit a directory.

Test case:

$ mkdir p1 p2 p1/foo
$ { echo '#!/bin/sh'; echo 'echo "foo found in p2"'; } > p2/foo
$ chmod a+x p2/foo
$ PATH=p1:p2:$PATH foo

Also, all shell follow symlinks. Test case:

$ mkdir p1 p2 p1/bar
$ ln -s bar p1/foo
$ { echo '#!/bin/sh'; echo 'echo "foo found in p2"'; } > p2/foo
$ chmod a+x p2/foo
$ PATH=p1:p2:$PATH foo
foo found in p2
$ sh -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ dash -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ bash -c 'PATH=p1:p2:$PATH foo'
foo found in p2

> +stat(progpathname, ) == 0 && S_ISREG(st.st_mode))

However, excluding other types of files (character devices, sockets, etc.)
is not what all shells do. Test cases:

1) For character devices:

$ ln -s bar p1/foo
$ sudo mknod p1/bar c 5 0
$ sudo chmod a+x p1/bar
$ dash -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ sh -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ bash -c 'PATH=p1:p2:$PATH foo'
(hangs reading)

2) For sockets:

On Linux:

$ mkdir p1 p2 p1/bar
$ ln -s /tmp/.X11-unix/X0 p1/foo
$ { echo '#!/bin/sh'; echo 'echo "foo found in p2"'; } > p2/foo
$ chmod a+x p2/foo
$ PATH=p1:p2:$PATH foo
$ dash -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ bash -c 'PATH=p1:p2:$PATH foo'
bash: p1/foo: No such device or address

and on Solaris:

$ ln -sf /var/run/.inetd.uds p1/foo
$ sh -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ ksh -c 'PATH=p1:p2:$PATH foo'
foo found in p2
$ bash -c 'PATH=p1:p2:$PATH foo'
bash: p1/foo: Permission denied

So, let me ignore directories and symlinks to directories only.

> The fix is likely needed for the windows specific piece of code in
> find_in_given_path as well.

Possibly, but I don't have time to make the necessary investigations on
Windows right now. Please feel free to make the investigations and provide
a patch, if you like to.


2020-03-07  Bruno Haible  

findprog, relocatable-prog: Ignore directories during PATH search.
Reported by Frederick Eaton via Dmitry Goncharov in
.
* lib/findprog.c (find_in_path): When the file found in a PATH element
is a directory, continue searching.
* lib/progreloc.c (maybe_executable): Likewise.

diff --git a/lib/findprog.c b/lib/findprog.c
index d0d4179..d834301 100644
--- a/lib/findprog.c
+++ b/lib/findprog.c
@@ -105,22 +105,29 @@ find_in_path (const char *progname)
  design flaw.  */
   if (eaccess (progpathname, X_OK) == 0)
 {
-  /* Found!  */
-  if (strcmp (progpathname, progname) == 0)
+  /* Check that the progpathname does not point to a directory.  */
+  struct stat statbuf;
+
+  if (stat (progpathname, ) >= 0
+  && ! S_ISDIR (statbuf.st_mode))
 {
-  free (progpathname);
-
-  /* Add the "./" prefix for real, that xconcatenated_filename()
- optimized away.  This avoids a second PATH search when the
- caller uses execlp/execvp.  */
-  progpathname = XNMALLOC (2 + strlen (progname) + 1, char);
-  progpathname[0] = '.';
-  progpathname[1] = '/';
-  memcpy (progpathname + 2, progname, strlen (progname) + 1);
+  /* Found!  */
+  if (strcmp (progpathname, progname) == 0)
+{
+  free (progpathname);
+
+  /* Add the "./" prefix for real, that 
xconcatenated_filename()
+ optimized away.  This avoids a second PATH search when the
+ caller uses execlp/execvp.  */
+  progpathname = XNMALLOC (2 + strlen (progname) + 1, char);
+  progpathname[0] = '.';
+  progpathname[1] = '/';
+  memcpy (progpathname + 2, progname, strlen (progname) + 1);
+}
+
+  free (path);
+  return progpathname;
 }
-
-  free (path);
-  return progpathname;
 }
 
   free (progpathname);
diff --git a/lib/progreloc.c b/lib/progreloc.c
index 2acf3fb..04cef32 100644
--- a/lib/progreloc.c
+++ b/lib/progreloc.c
@@ -154,7 +154,7 @@ static int executable_fd = -1;
 /* Define this function only when it's needed.  */
 #if !(defined WINDOWS_NATIVE || defined __EMX__)
 
-/* Tests whether a given pathname may belong to the executable.  */
+/* Tests whether 

Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Ben Pfaff]

On Sat, Mar 7, 2020 at 9:36 AM Bruno Haible  wrote:
>
> Dan Gohman wrote:
> > That would fix the problem for systems that define O_RDONLY as Hurd does,
> > while not breaking any known systems.
> >
> > It wouldn't be correct on a hypothetical system which defines the constants
> > like this:
> >
> > #define O_WRONLY 0
> > #define O_RDONLY 1
> > #define O_RDWR 2
> >
> > though I don't know of any platforms which do this.
>
> Reviewing the include files of the various platforms shows that
> such a hypothetical system doesn't exist:
>
>  O_RDONLY   O_WRONLY   O_RDWR
>
> AIX
> Android
> BeOS
> Cygwin
> FreeBSD
> glibc/Linux
> Haiku
> HP-UX
> Interix
> IRIX
> macOS
> mingw, MSVC
> Minix
> MirBSD
> musl
> NetBSD
> OpenBSD
> OSF/1
> pips
> Plan 9
> Solaris
>  0  1 2
>
> glibc/Hurd   1  2 3

Is there some reason that O_ACCMODE isn't the best choice here?

Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Bruno Haible]

Dan Gohman wrote:
> That would fix the problem for systems that define O_RDONLY as Hurd does,
> while not breaking any known systems.
> 
> It wouldn't be correct on a hypothetical system which defines the constants
> like this:
> 
> #define O_WRONLY 0
> #define O_RDONLY 1
> #define O_RDWR 2
> 
> though I don't know of any platforms which do this.

Reviewing the include files of the various platforms shows that
such a hypothetical system doesn't exist:

 O_RDONLY   O_WRONLY   O_RDWR

AIX
Android
BeOS
Cygwin
FreeBSD
glibc/Linux
Haiku
HP-UX
Interix
IRIX
macOS
mingw, MSVC
Minix
MirBSD
musl
NetBSD
OpenBSD
OSF/1
pips
Plan 9
Solaris
 0  1 2

glibc/Hurd   1  2 3


So this patch should be safe:


2020-03-07  Bruno Haible  

openat: Fix theoretically possible issue on GNU/Hurd.
Reported by Dan Gohman  in
.
* lib/openat.c (rpl_openat): When testing whether flags contains O_RDWR,
ignore the bits that are also set in O_RDONLY.

diff --git a/lib/openat.c b/lib/openat.c
index d2c84e8..7d67dc4 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -100,7 +100,7 @@ rpl_openat (int dfd, char const *filename, int flags, ...)
  directories,
- if O_WRONLY or O_RDWR is specified, open() must fail because the
  file does not contain a '.' directory.  */
-  if (flags & (O_CREAT | O_WRONLY | O_RDWR))
+  if (flags & (O_CREAT | O_WRONLY | (O_RDWR & ~O_RDONLY)))
 {
   size_t len = strlen (filename);
   if (len > 0 && filename[len - 1] == '/')