* openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
@ 2020-03-03 20:24 Dan Gohman
2020-03-04 2:47 ` Bruno Haible
0 siblings, 1 reply; 7+ messages in thread
From: Dan Gohman @ 2020-03-03 20:24 UTC (permalink / raw)
To: bug-gnulib
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-03 20:24 openat wrapper assumes that O_RDONLY is disjoint from R_RDWR Dan Gohman
@ 2020-03-04 2:47 ` Bruno Haible
2020-03-04 17:24 ` Dan Gohman
0 siblings, 1 reply; 7+ messages in thread
From: Bruno Haible @ 2020-03-04 2:47 UTC (permalink / raw)
To: bug-gnulib; +Cc: Dan Gohman
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-04 2:47 ` Bruno Haible
@ 2020-03-04 17:24 ` Dan Gohman
2020-03-07 17:36 ` Bruno Haible
2020-03-07 19:05 ` Paul Eggert
0 siblings, 2 replies; 7+ messages in thread
From: Dan Gohman @ 2020-03-04 17:24 UTC (permalink / raw)
To: Bruno Haible; +Cc: bug-gnulib
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-04 17:24 ` Dan Gohman
@ 2020-03-07 17:36 ` Bruno Haible
2020-03-07 18:13 ` Ben Pfaff
2020-03-07 19:05 ` Paul Eggert
1 sibling, 1 reply; 7+ messages in thread
From: Bruno Haible @ 2020-03-07 17:36 UTC (permalink / raw)
To: Dan Gohman; +Cc: bug-gnulib
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] == '/')
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-07 17:36 ` Bruno Haible
@ 2020-03-07 18:13 ` Ben Pfaff
0 siblings, 0 replies; 7+ messages in thread
From: Ben Pfaff @ 2020-03-07 18:13 UTC (permalink / raw)
To: Bruno Haible; +Cc: bug-gnulib, Dan Gohman
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-04 17:24 ` Dan Gohman
2020-03-07 17:36 ` Bruno Haible
@ 2020-03-07 19:05 ` Paul Eggert
2020-03-08 1:58 ` Bruno Haible
1 sibling, 1 reply; 7+ messages in thread
From: Paul Eggert @ 2020-03-07 19:05 UTC (permalink / raw)
To: Dan Gohman, Bruno Haible; +Cc: bug-gnulib
[-- 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: openat wrapper assumes that O_RDONLY is disjoint from R_RDWR
2020-03-07 19:05 ` Paul Eggert
@ 2020-03-08 1:58 ` Bruno Haible
0 siblings, 0 replies; 7+ messages in thread
From: Bruno Haible @ 2020-03-08 1:58 UTC (permalink / raw)
To: Paul Eggert; +Cc: bug-gnulib, Dan Gohman
Paul Eggert wrote:
> we can use O_ACCMODE here.
Your patch is obviously better than my previous one. Thanks.
Bruno
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-08 1:58 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 20:24 openat wrapper assumes that O_RDONLY is disjoint from R_RDWR Dan Gohman
2020-03-04 2:47 ` Bruno Haible
2020-03-04 17:24 ` Dan Gohman
2020-03-07 17:36 ` Bruno Haible
2020-03-07 18:13 ` Ben Pfaff
2020-03-07 19:05 ` Paul Eggert
2020-03-08 1:58 ` Bruno Haible
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).