[2/6] Open a directory with the usual flags

Message ID 20181008133900.3554-2-sebastian.huber@embedded-brains.de
State Accepted
Commit 61fc64ed975d0c47582c23e5ce47a4af51c66733
Headers show
Series
  • [1/6] O_CLOEXEC O_NOFOLLOW O_DIRECTORY O_EXEC O_DIRECT
Related show

Commit Message

Sebastian Huber Oct. 8, 2018, 1:38 p.m.
Use O_RDONLY since you are not supposed to write to a directory.

Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications
Issue 7, 2018 edition IEEE Std 1003.1-2017):

"If the type DIR is implemented using a file descriptor, the descriptor
shall be obtained as if the O_DIRECTORY flag was passed to open()."

Use O_CLOEXEC as mandated by POSIX:

"When a file descriptor is used to implement the directory stream, it
behaves as if the FD_CLOEXEC had been set for the file descriptor."

Drop the fcntl() call in favour of O_CLOEXEC.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>

---
 newlib/libc/posix/opendir.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

-- 
2.16.4

Comments

Corinna Vinschen Oct. 10, 2018, 9:50 a.m. | #1
On Oct  8 15:38, Sebastian Huber wrote:
> Use O_RDONLY since you are not supposed to write to a directory.

> 

> Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications

> Issue 7, 2018 edition IEEE Std 1003.1-2017):

> 

> "If the type DIR is implemented using a file descriptor, the descriptor

> shall be obtained as if the O_DIRECTORY flag was passed to open()."

> 

> Use O_CLOEXEC as mandated by POSIX:

> 

> "When a file descriptor is used to implement the directory stream, it

> behaves as if the FD_CLOEXEC had been set for the file descriptor."

> 

> Drop the fcntl() call in favour of O_CLOEXEC.


Yeah, that really makes sense, but what about targets not (yet)
implementing O_CLOEXEC???  I'm not sure how to handle this, if
we have to handle that at all.


Thoughts?
Corinna


> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>

> ---

>  newlib/libc/posix/opendir.c | 13 ++++---------

>  1 file changed, 4 insertions(+), 9 deletions(-)

> 

> diff --git a/newlib/libc/posix/opendir.c b/newlib/libc/posix/opendir.c

> index 1416f1053..650cbfe8d 100644

> --- a/newlib/libc/posix/opendir.c

> +++ b/newlib/libc/posix/opendir.c

> @@ -49,17 +49,12 @@ static char sccsid[] = "@(#)opendir.c	5.11 (Berkeley) 2/23/91";

>  DIR *

>  opendir (const char *name)

>  {

> -	register DIR *dirp;

> -	register int fd;

> -	int rc = 0;

> +	DIR *dirp;

> +	int fd;

>  

> -	if ((fd = open(name, 0)) == -1)

> +	if ((fd = open(name, O_RDONLY | O_DIRECTORY | O_CLOEXEC)) == -1)

>  		return NULL;

> -#ifdef HAVE_FCNTL

> -	rc = fcntl(fd, F_SETFD, 1);

> -#endif

> -	if (rc == -1 ||

> -	    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {

> +	if ((dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {

>  		close (fd);

>  		return NULL;

>  	}

> -- 

> 2.16.4


-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Sebastian Huber Oct. 10, 2018, 11:16 a.m. | #2
On 10/10/2018 11:50, Corinna Vinschen wrote:
> On Oct  8 15:38, Sebastian Huber wrote:

>> Use O_RDONLY since you are not supposed to write to a directory.

>>

>> Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications

>> Issue 7, 2018 edition IEEE Std 1003.1-2017):

>>

>> "If the type DIR is implemented using a file descriptor, the descriptor

>> shall be obtained as if the O_DIRECTORY flag was passed to open()."

>>

>> Use O_CLOEXEC as mandated by POSIX:

>>

>> "When a file descriptor is used to implement the directory stream, it

>> behaves as if the FD_CLOEXEC had been set for the file descriptor."

>>

>> Drop the fcntl() call in favour of O_CLOEXEC.

> Yeah, that really makes sense, but what about targets not (yet)

> implementing O_CLOEXEC???  I'm not sure how to handle this, if

> we have to handle that at all.


In RTEMS there are no processes (fork(), exec(), etc. don't work). So, 
it is trivial to support the O_CLOEXEC. The situation in which this flag 
is relevant simply cannot happen.

Are Cygwin and Linux the only Newlib systems which supports processes?

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Corinna Vinschen Oct. 10, 2018, 11:35 a.m. | #3
On Oct 10 13:16, Sebastian Huber wrote:
> On 10/10/2018 11:50, Corinna Vinschen wrote:

> > On Oct  8 15:38, Sebastian Huber wrote:

> > > Use O_RDONLY since you are not supposed to write to a directory.

> > > 

> > > Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications

> > > Issue 7, 2018 edition IEEE Std 1003.1-2017):

> > > 

> > > "If the type DIR is implemented using a file descriptor, the descriptor

> > > shall be obtained as if the O_DIRECTORY flag was passed to open()."

> > > 

> > > Use O_CLOEXEC as mandated by POSIX:

> > > 

> > > "When a file descriptor is used to implement the directory stream, it

> > > behaves as if the FD_CLOEXEC had been set for the file descriptor."

> > > 

> > > Drop the fcntl() call in favour of O_CLOEXEC.

> > Yeah, that really makes sense, but what about targets not (yet)

> > implementing O_CLOEXEC???  I'm not sure how to handle this, if

> > we have to handle that at all.

> 

> In RTEMS there are no processes (fork(), exec(), etc. don't work). So, it is

> trivial to support the O_CLOEXEC. The situation in which this flag is

> relevant simply cannot happen.

> 

> Are Cygwin and Linux the only Newlib systems which supports processes?


I really don't know, but the other problem is to use a flag in a target
dependent call which might be unsupported, but tested.  This is backed
by POSIX allowing open to return EINVAL for invalid flags.

We should probably define _FNOINHERIT only on targets known to support
it (even if trivially) and to call open(O_CLOEXEC) or fcntl() depending
on that test, no?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Sebastian Huber Oct. 10, 2018, 11:55 a.m. | #4
On 10/10/2018 13:35, Corinna Vinschen wrote:
> On Oct 10 13:16, Sebastian Huber wrote:

>> On 10/10/2018 11:50, Corinna Vinschen wrote:

>>> On Oct  8 15:38, Sebastian Huber wrote:

>>>> Use O_RDONLY since you are not supposed to write to a directory.

>>>>

>>>> Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications

>>>> Issue 7, 2018 edition IEEE Std 1003.1-2017):

>>>>

>>>> "If the type DIR is implemented using a file descriptor, the descriptor

>>>> shall be obtained as if the O_DIRECTORY flag was passed to open()."

>>>>

>>>> Use O_CLOEXEC as mandated by POSIX:

>>>>

>>>> "When a file descriptor is used to implement the directory stream, it

>>>> behaves as if the FD_CLOEXEC had been set for the file descriptor."

>>>>

>>>> Drop the fcntl() call in favour of O_CLOEXEC.

>>> Yeah, that really makes sense, but what about targets not (yet)

>>> implementing O_CLOEXEC???  I'm not sure how to handle this, if

>>> we have to handle that at all.

>> In RTEMS there are no processes (fork(), exec(), etc. don't work). So, it is

>> trivial to support the O_CLOEXEC. The situation in which this flag is

>> relevant simply cannot happen.

>>

>> Are Cygwin and Linux the only Newlib systems which supports processes?

> I really don't know, but the other problem is to use a flag in a target

> dependent call which might be unsupported, but tested.  This is backed

> by POSIX allowing open to return EINVAL for invalid flags.

>

> We should probably define _FNOINHERIT only on targets known to support

> it (even if trivially) and to call open(O_CLOEXEC) or fcntl() depending

> on that test, no?


POSIX says there is an O_CLOEXEC, so I think this is a valid oflag 
value. An unsupported close on execute would be more an ENOSYS which is 
not mentioned in the error list:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

I would define the POSIX flags unconditionally and in case errors show 
up on a particular system, then we can find a solution, e.g. not define 
it on system X.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Corinna Vinschen Oct. 10, 2018, 1:21 p.m. | #5
On Oct 10 13:55, Sebastian Huber wrote:
> On 10/10/2018 13:35, Corinna Vinschen wrote:

> > On Oct 10 13:16, Sebastian Huber wrote:

> > > On 10/10/2018 11:50, Corinna Vinschen wrote:

> > > > On Oct  8 15:38, Sebastian Huber wrote:

> > > > > Use O_RDONLY since you are not supposed to write to a directory.

> > > > > 

> > > > > Use O_DIRECTORY as mandated by POSIX (The Open Group Base Specifications

> > > > > Issue 7, 2018 edition IEEE Std 1003.1-2017):

> > > > > 

> > > > > "If the type DIR is implemented using a file descriptor, the descriptor

> > > > > shall be obtained as if the O_DIRECTORY flag was passed to open()."

> > > > > 

> > > > > Use O_CLOEXEC as mandated by POSIX:

> > > > > 

> > > > > "When a file descriptor is used to implement the directory stream, it

> > > > > behaves as if the FD_CLOEXEC had been set for the file descriptor."

> > > > > 

> > > > > Drop the fcntl() call in favour of O_CLOEXEC.

> > > > Yeah, that really makes sense, but what about targets not (yet)

> > > > implementing O_CLOEXEC???  I'm not sure how to handle this, if

> > > > we have to handle that at all.

> > > In RTEMS there are no processes (fork(), exec(), etc. don't work). So, it is

> > > trivial to support the O_CLOEXEC. The situation in which this flag is

> > > relevant simply cannot happen.

> > > 

> > > Are Cygwin and Linux the only Newlib systems which supports processes?

> > I really don't know, but the other problem is to use a flag in a target

> > dependent call which might be unsupported, but tested.  This is backed

> > by POSIX allowing open to return EINVAL for invalid flags.

> > 

> > We should probably define _FNOINHERIT only on targets known to support

> > it (even if trivially) and to call open(O_CLOEXEC) or fcntl() depending

> > on that test, no?

> 

> POSIX says there is an O_CLOEXEC, so I think this is a valid oflag value. An

> unsupported close on execute would be more an ENOSYS which is not mentioned

> in the error list:

> 

> http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

> 

> I would define the POSIX flags unconditionally and in case errors show up on

> a particular system, then we can find a solution, e.g. not define it on

> system X.


Ok, ACK.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Patch

diff --git a/newlib/libc/posix/opendir.c b/newlib/libc/posix/opendir.c
index 1416f1053..650cbfe8d 100644
--- a/newlib/libc/posix/opendir.c
+++ b/newlib/libc/posix/opendir.c
@@ -49,17 +49,12 @@  static char sccsid[] = "@(#)opendir.c	5.11 (Berkeley) 2/23/91";
 DIR *
 opendir (const char *name)
 {
-	register DIR *dirp;
-	register int fd;
-	int rc = 0;
+	DIR *dirp;
+	int fd;
 
-	if ((fd = open(name, 0)) == -1)
+	if ((fd = open(name, O_RDONLY | O_DIRECTORY | O_CLOEXEC)) == -1)
 		return NULL;
-#ifdef HAVE_FCNTL
-	rc = fcntl(fd, F_SETFD, 1);
-#endif
-	if (rc == -1 ||
-	    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
+	if ((dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
 		close (fd);
 		return NULL;
 	}