openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Dan Gohman]

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

The following code in Gnulib's openat function:

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/openat.c;h=d2c84e8f80489201be2a7198bb979fe4be6f9ecb;hb=HEAD#l103

tests for a file mode being writable with the following expression:

> if (flags & (O_CREAT | O_WRONLY | O_RDWR))

However, this can erroneously return true on systems where O_RDWR shares
bits with R_RDONLY. POSIX doesn't guarantee anything about the values of
O_RDWR and O_RDONLY, and GNU Hurd is an example of a system where O_RDWR
and O_RDONLY do share bits:

https://www.gnu.org/software/libc/manual/html_node/Access-Modes.html#Access-Modes

That said, the code in question is only used on platforms where
`OPEN_TRAILING_SLASH_BUG` is defined, and I don't have any ready examples
of relevant platforms with both `OPEN_TRAILING_SLASH_BUG` defined and with
`O_RDWR` sharing bits with `O_RDONLY`. However, such a platform is a
theoretical possibility.

Dan

[-- Attachment #2: Type: text/html, Size: 1335 bytes --]

Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Bruno Haible]

Dan Gohman wrote:
> > if (flags & (O_CREAT | O_WRONLY | O_RDWR))
> 
> However, this can erroneously return true on systems where O_RDWR shares
> bits with R_RDONLY.

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

Bruno

Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR

[Dan Gohman]

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

On Tue, Mar 3, 2020 at 6:47 PM Bruno Haible <bruno@clisp.org> wrote:

> Dan Gohman wrote:
> > > if (flags & (O_CREAT | O_WRONLY | O_RDWR))
> >
> > However, this can erroneously return true on systems where O_RDWR shares
> > bits with R_RDONLY.
>
> 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 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.

Dan

[-- Attachment #2: Type: text/html, Size: 1175 bytes --]

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  <bruno@clisp.org>

	openat: Fix theoretically possible issue on GNU/Hurd.
	Reported by Dan Gohman <sunfish@mozilla.com> in
	<https://lists.gnu.org/archive/html/bug-gnulib/2020-03/msg00000.html>.
	* 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] == '/')

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 <bruno@clisp.org> 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

[Paul Eggert]

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

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.

[-- Attachment #2: 0001-open-openat-port-to-O_RDWR-O_RDONLY-0.patch --]
[-- Type: text/x-patch, Size: 2456 bytes --]

From 1d86584739a712597a6fe3a62ec412238f0f13f8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
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/msg00000.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  <eggert@cs.ucla.edu>
+
+	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/msg00000.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  <bruno@clisp.org>
 
 	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: 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