[RFC PATCH v2 0/7] O_IGNORE_CTTY everywhere & misc fixes

[RFC PATCH v2 0/7] O_IGNORE_CTTY everywhere & misc fixes

[Sergey Bugaev]

Changes since v1:
* Addressed nits
* Made improvements to daemon () as discussed
* Fixed std{in,out,err} mode in csu/check_fds.c
* Tweaked the comment on O_IGNORE_CTTY

But also, I found out that opening some internal files (specifically
I have noticed this about nsswitch.conf) still triggers the
term_getctty () RPC -- and that's because they're opened with
fopen (), not plain open (). Is there anything we can do abiut this?

I suppose there's no way we're could introduce a new fopen () mode
character just for this?

Even with the fopen-using callsites left out, not making ctty RPCs in
all the other cases is still a solid improvement.

Sergey

Sergey Bugaev (7):
  misc: Convert daemon () to GNU coding style
  misc: Ignore SIGHUP in daemon () while forking
  Use O_CLOEXEC in more places (BZ #15722)
  csu: Fix standard fds' mode
  hurd: Make dl-sysdep's open () cope with O_IGNORE_CTTY
  include/fcntl.h: Define O_IGNORE_CTTY
  Use O_IGNORE_CTTY where appropriate

 catgets/open_catalog.c        |   4 +-
 csu/check_fds.c               |   6 +-
 elf/dl-load.c                 |   2 +-
 elf/dl-misc.c                 |   2 +-
 elf/dl-profile.c              |   3 +-
 gmon/gmon.c                   |   7 ++-
 iconv/gconv_cache.c           |   3 +-
 include/fcntl.h               |  15 +++++
 locale/loadarchive.c          |   7 ++-
 locale/loadlocale.c           |   4 +-
 login/openpty.c               |   2 +-
 login/utmp_file.c             |   7 ++-
 misc/daemon.c                 | 103 +++++++++++++++++++++-------------
 nss/nss_db/db-open.c          |   3 +-
 rt/shm_open.c                 |   2 +-
 shadow/lckpwdf.c              |   2 +-
 sysdeps/mach/hurd/dl-sysdep.c |   4 +-
 sysdeps/mach/hurd/opendir.c   |   2 +-
 sysdeps/pthread/sem_open.c    |   9 ++-
 sysdeps/unix/bsd/getpt.c      |   4 +-
 20 files changed, 120 insertions(+), 71 deletions(-)

-- 
2.40.0

[RFC PATCH v2 1/7] misc: Convert daemon () to GNU coding style

[Sergey Bugaev]

This is nicer, and is going to be required for the following changes
to reasonably stay within the 79 column limit.

No functional change.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/misc/daemon.c b/misc/daemon.c
index 3c73ac2a..14577e40 100644
--- a/misc/daemon.c
+++ b/misc/daemon.c
@@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
 int
 daemon (int nochdir, int noclose)
 {
-	int fd;
+  int fd;
 
-	switch (__fork()) {
-	case -1:
-		return (-1);
-	case 0:
-		break;
-	default:
-		_exit(0);
-	}
+  switch (__fork ())
+    {
+    case -1:
+      return -1;
 
-	if (__setsid() == -1)
-		return (-1);
+    case 0:
+      break;
 
-	if (!nochdir)
-		(void)__chdir("/");
+    default:
+      _exit (0);
+    }
 
-	if (!noclose) {
-		struct __stat64_t64 st;
+  if (__setsid () == -1)
+    return -1;
 
-		if ((fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0)) != -1
-		    && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) {
-			if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
+  if (!nochdir)
+    (void) __chdir ("/");
+
+  if (!noclose)
+    {
+      struct __stat64_t64 st;
+
+      fd = __open_nocancel (_PATH_DEVNULL, O_RDWR, 0);
+      if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0))
+        {
+          if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
 #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR
-			    && (st.st_rdev
-				== makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
+              && (st.st_rdev == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
 #endif
-			    ) {
-				(void)__dup2(fd, STDIN_FILENO);
-				(void)__dup2(fd, STDOUT_FILENO);
-				(void)__dup2(fd, STDERR_FILENO);
-				if (fd > 2)
-					(void)__close (fd);
-			} else {
-				/* We must set an errno value since no
-				   function call actually failed.  */
-				__close_nocancel_nostatus (fd);
-				__set_errno (ENODEV);
-				return -1;
-			}
-		} else {
-			__close_nocancel_nostatus (fd);
-			return -1;
-		}
-	}
-	return (0);
+             )
+            {
+              (void) __dup2 (fd, STDIN_FILENO);
+              (void) __dup2 (fd, STDOUT_FILENO);
+              (void) __dup2 (fd, STDERR_FILENO);
+              if (fd > 2)
+                (void) __close (fd);
+            }
+          else
+            {
+              /* We must set an errno value since no function call
+                 actually failed.  */
+              __close_nocancel_nostatus (fd);
+              __set_errno (ENODEV);
+              return -1;
+            }
+        }
+      else
+        {
+          __close_nocancel_nostatus (fd);
+          return -1;
+        }
+    }
+
+  return 0;
 }
-- 
2.40.0

Re: [RFC PATCH v2 1/7] misc: Convert daemon () to GNU coding style

[Adhemerval Zanella Netto]

On 19/04/23 13:02, Sergey Bugaev wrote:
> This is nicer, and is going to be required for the following changes
> to reasonably stay within the 79 column limit.
> 
> No functional change.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

LGTM, thanks.  I can be installed independently.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/misc/daemon.c b/misc/daemon.c
> index 3c73ac2a..14577e40 100644
> --- a/misc/daemon.c
> +++ b/misc/daemon.c
> @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
>  int
>  daemon (int nochdir, int noclose)
>  {
> -	int fd;
> +  int fd;
>  
> -	switch (__fork()) {
> -	case -1:
> -		return (-1);
> -	case 0:
> -		break;
> -	default:
> -		_exit(0);
> -	}
> +  switch (__fork ())
> +    {
> +    case -1:
> +      return -1;
>  
> -	if (__setsid() == -1)
> -		return (-1);
> +    case 0:
> +      break;
>  
> -	if (!nochdir)
> -		(void)__chdir("/");
> +    default:
> +      _exit (0);
> +    }
>  
> -	if (!noclose) {
> -		struct __stat64_t64 st;
> +  if (__setsid () == -1)
> +    return -1;
>  
> -		if ((fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0)) != -1
> -		    && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) {
> -			if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
> +  if (!nochdir)
> +    (void) __chdir ("/");
> +
> +  if (!noclose)
> +    {
> +      struct __stat64_t64 st;
> +
> +      fd = __open_nocancel (_PATH_DEVNULL, O_RDWR, 0);
> +      if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0))
> +        {
> +          if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
>  #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR
> -			    && (st.st_rdev
> -				== makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
> +              && (st.st_rdev == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
>  #endif
> -			    ) {
> -				(void)__dup2(fd, STDIN_FILENO);
> -				(void)__dup2(fd, STDOUT_FILENO);
> -				(void)__dup2(fd, STDERR_FILENO);
> -				if (fd > 2)
> -					(void)__close (fd);
> -			} else {
> -				/* We must set an errno value since no
> -				   function call actually failed.  */
> -				__close_nocancel_nostatus (fd);
> -				__set_errno (ENODEV);
> -				return -1;
> -			}
> -		} else {
> -			__close_nocancel_nostatus (fd);
> -			return -1;
> -		}
> -	}
> -	return (0);
> +             )
> +            {
> +              (void) __dup2 (fd, STDIN_FILENO);
> +              (void) __dup2 (fd, STDOUT_FILENO);
> +              (void) __dup2 (fd, STDERR_FILENO);
> +              if (fd > 2)
> +                (void) __close (fd);
> +            }
> +          else
> +            {
> +              /* We must set an errno value since no function call
> +                 actually failed.  */
> +              __close_nocancel_nostatus (fd);
> +              __set_errno (ENODEV);
> +              return -1;
> +            }
> +        }
> +      else
> +        {
> +          __close_nocancel_nostatus (fd);
> +          return -1;
> +        }
> +    }
> +
> +  return 0;
>  }

Re: [RFC PATCH v2 1/7] misc: Convert daemon () to GNU coding style

[Samuel Thibault]

Applied, thanks!

Adhemerval Zanella Netto, le ven. 21 avril 2023 09:18:19 -0300, a ecrit:
> 
> 
> On 19/04/23 13:02, Sergey Bugaev wrote:
> > This is nicer, and is going to be required for the following changes
> > to reasonably stay within the 79 column limit.
> > 
> > No functional change.
> > 
> > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> 
> LGTM, thanks.  I can be installed independently.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> > ---
> >  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 49 insertions(+), 39 deletions(-)
> > 
> > diff --git a/misc/daemon.c b/misc/daemon.c
> > index 3c73ac2a..14577e40 100644
> > --- a/misc/daemon.c
> > +++ b/misc/daemon.c
> > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
> >  int
> >  daemon (int nochdir, int noclose)
> >  {
> > -	int fd;
> > +  int fd;
> >  
> > -	switch (__fork()) {
> > -	case -1:
> > -		return (-1);
> > -	case 0:
> > -		break;
> > -	default:
> > -		_exit(0);
> > -	}
> > +  switch (__fork ())
> > +    {
> > +    case -1:
> > +      return -1;
> >  
> > -	if (__setsid() == -1)
> > -		return (-1);
> > +    case 0:
> > +      break;
> >  
> > -	if (!nochdir)
> > -		(void)__chdir("/");
> > +    default:
> > +      _exit (0);
> > +    }
> >  
> > -	if (!noclose) {
> > -		struct __stat64_t64 st;
> > +  if (__setsid () == -1)
> > +    return -1;
> >  
> > -		if ((fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0)) != -1
> > -		    && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) {
> > -			if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
> > +  if (!nochdir)
> > +    (void) __chdir ("/");
> > +
> > +  if (!noclose)
> > +    {
> > +      struct __stat64_t64 st;
> > +
> > +      fd = __open_nocancel (_PATH_DEVNULL, O_RDWR, 0);
> > +      if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0))
> > +        {
> > +          if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
> >  #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR
> > -			    && (st.st_rdev
> > -				== makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
> > +              && (st.st_rdev == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR))
> >  #endif
> > -			    ) {
> > -				(void)__dup2(fd, STDIN_FILENO);
> > -				(void)__dup2(fd, STDOUT_FILENO);
> > -				(void)__dup2(fd, STDERR_FILENO);
> > -				if (fd > 2)
> > -					(void)__close (fd);
> > -			} else {
> > -				/* We must set an errno value since no
> > -				   function call actually failed.  */
> > -				__close_nocancel_nostatus (fd);
> > -				__set_errno (ENODEV);
> > -				return -1;
> > -			}
> > -		} else {
> > -			__close_nocancel_nostatus (fd);
> > -			return -1;
> > -		}
> > -	}
> > -	return (0);
> > +             )
> > +            {
> > +              (void) __dup2 (fd, STDIN_FILENO);
> > +              (void) __dup2 (fd, STDOUT_FILENO);
> > +              (void) __dup2 (fd, STDERR_FILENO);
> > +              if (fd > 2)
> > +                (void) __close (fd);
> > +            }
> > +          else
> > +            {
> > +              /* We must set an errno value since no function call
> > +                 actually failed.  */
> > +              __close_nocancel_nostatus (fd);
> > +              __set_errno (ENODEV);
> > +              return -1;
> > +            }
> > +        }
> > +      else
> > +        {
> > +          __close_nocancel_nostatus (fd);
> > +          return -1;
> > +        }
> > +    }
> > +
> > +  return 0;
> >  }
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

[RFC PATCH v2 2/7] misc: Ignore SIGHUP in daemon () while forking

[Sergey Bugaev]

Under certain conditions, SIGHUP will be sent to the child process when
the parent process (the one calling daemon ()) terminates -- namely, if
the parent process was the session leader and the child process was in
the same session (which will be the case after fork and until setsid)
and in the foreground process group. To prevent this SIGHUP from killing
the child, temporarily ignore it. Once we leave the parent's session, we
can restore the original SIGHUP sigaction.

Or that's what you'd hope would happen. Unfortunately for us, nothing
guarantess that signal delivery is synchronous. This means that a SIGHUP
may be generated and enqueued for the child process while it's still a
member of the original session, but only delivered to it some time
later, once it has already left the session and restored the original
sigaction.

Still, on many systems signal delivery is synchronous enough, and all
pending signals will get reliably delivered upon performing a syscall,
specifically setsid () in this case, so this change is still worth it.

Also, do not ignore erros from chdir ("/").

Suggested-by: Cristian Rodríguez <crrodriguez@opensuse.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 misc/daemon.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/misc/daemon.c b/misc/daemon.c
index 14577e40..58dde4f0 100644
--- a/misc/daemon.c
+++ b/misc/daemon.c
@@ -35,6 +35,7 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
 #include <fcntl.h>
 #include <paths.h>
 #include <unistd.h>
+#include <signal.h>
 #include <sys/stat.h>
 
 #include <device-nrs.h>
@@ -44,6 +45,14 @@ int
 daemon (int nochdir, int noclose)
 {
   int fd;
+  int set_sigaction;
+  struct sigaction act, oact;
+
+  /* When the parent process exits, the child might get a SIGHUP if the parent
+     was a session leader.  Arrange things so that it doesn't terminate us.  */
+  memset (&act, 0, sizeof (act));
+  act.sa_handler = SIG_IGN;
+  set_sigaction = __libc_sigaction (SIGHUP, &act, &oact) == 0;
 
   switch (__fork ())
     {
@@ -60,8 +69,14 @@ daemon (int nochdir, int noclose)
   if (__setsid () == -1)
     return -1;
 
+  /* Now that we have left the parent's session, we should no longer be at
+     risk of receiving SIGHUP because of the parent process exiting.  */
+  if (__glibc_likely (set_sigaction))
+    __libc_sigaction (SIGHUP, &oact, NULL);
+
   if (!nochdir)
-    (void) __chdir ("/");
+    if (__glibc_unlikely (__chdir ("/") == -1))
+      return -1;
 
   if (!noclose)
     {
-- 
2.40.0

Re: [RFC PATCH v2 2/7] misc: Ignore SIGHUP in daemon () while forking

[Adhemerval Zanella Netto]

On 19/04/23 13:02, Sergey Bugaev wrote:
> Under certain conditions, SIGHUP will be sent to the child process when
> the parent process (the one calling daemon ()) terminates -- namely, if
> the parent process was the session leader and the child process was in
> the same session (which will be the case after fork and until setsid)
> and in the foreground process group. To prevent this SIGHUP from killing
> the child, temporarily ignore it. Once we leave the parent's session, we
> can restore the original SIGHUP sigaction.
> 
> Or that's what you'd hope would happen. Unfortunately for us, nothing
> guarantess that signal delivery is synchronous. This means that a SIGHUP
> may be generated and enqueued for the child process while it's still a
> member of the original session, but only delivered to it some time
> later, once it has already left the session and restored the original
> sigaction.
> 
> Still, on many systems signal delivery is synchronous enough, and all
> pending signals will get reliably delivered upon performing a syscall,
> specifically setsid () in this case, so this change is still worth it.
> 
> Also, do not ignore erros from chdir ("/").
> 
> Suggested-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

Wouldn't a double fork avoid binding the daemon process to terminal to
reacquire a controlling terminal, thus avoid SIGHUP? This is BZ#19144 [1],
and this is being used by various libc implementations.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=19144

> ---
>  misc/daemon.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/daemon.c b/misc/daemon.c
> index 14577e40..58dde4f0 100644
> --- a/misc/daemon.c
> +++ b/misc/daemon.c
> @@ -35,6 +35,7 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
>  #include <fcntl.h>
>  #include <paths.h>
>  #include <unistd.h>
> +#include <signal.h>
>  #include <sys/stat.h>
>  
>  #include <device-nrs.h>
> @@ -44,6 +45,14 @@ int
>  daemon (int nochdir, int noclose)
>  {
>    int fd;
> +  int set_sigaction;
> +  struct sigaction act, oact;
> +
> +  /* When the parent process exits, the child might get a SIGHUP if the parent
> +     was a session leader.  Arrange things so that it doesn't terminate us.  */
> +  memset (&act, 0, sizeof (act));
> +  act.sa_handler = SIG_IGN;
> +  set_sigaction = __libc_sigaction (SIGHUP, &act, &oact) == 0;
>  
>    switch (__fork ())
>      {
> @@ -60,8 +69,14 @@ daemon (int nochdir, int noclose)
>    if (__setsid () == -1)
>      return -1;
>  
> +  /* Now that we have left the parent's session, we should no longer be at
> +     risk of receiving SIGHUP because of the parent process exiting.  */
> +  if (__glibc_likely (set_sigaction))
> +    __libc_sigaction (SIGHUP, &oact, NULL);
> +
>    if (!nochdir)
> -    (void) __chdir ("/");
> +    if (__glibc_unlikely (__chdir ("/") == -1))
> +      return -1;
>  
>    if (!noclose)
>      {

[RFC PATCH v2 3/7] Use O_CLOEXEC in more places (BZ #15722)

[Sergey Bugaev]

When opening a temporary file without O_CLOEXEC we risk leaking the
file descriptor if another thread calls (fork and then) exec while we
have the fd open. Fix this by consistently passing O_CLOEXEC everywhere
where we open a file for internal use (and not to return it to the user,
in which case the API defines whether or not the close-on-exec flag
shall be set on the returned fd).

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 catgets/open_catalog.c     | 4 ++--
 elf/dl-profile.c           | 3 ++-
 gmon/gmon.c                | 7 ++++---
 iconv/gconv_cache.c        | 2 +-
 login/utmp_file.c          | 2 +-
 sysdeps/pthread/sem_open.c | 9 ++++++---
 6 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
index 242709db..46c444d2 100644
--- a/catgets/open_catalog.c
+++ b/catgets/open_catalog.c
@@ -49,7 +49,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
   char *buf = NULL;
 
   if (strchr (cat_name, '/') != NULL || nlspath == NULL)
-    fd = __open_nocancel (cat_name, O_RDONLY);
+    fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC);
   else
     {
       const char *run_nlspath = nlspath;
@@ -177,7 +177,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
 
 	  if (bufact != 0)
 	    {
-	      fd = __open_nocancel (buf, O_RDONLY);
+	      fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC);
 	      if (fd >= 0)
 		break;
 	    }
diff --git a/elf/dl-profile.c b/elf/dl-profile.c
index 2ecac05f..d8345da2 100644
--- a/elf/dl-profile.c
+++ b/elf/dl-profile.c
@@ -324,7 +324,8 @@ _dl_start_profile (void)
   *cp++ = '/';
   __stpcpy (__stpcpy (cp, GLRO(dl_profile)), ".profile");
 
-  fd = __open64_nocancel (filename, O_RDWR|O_CREAT|O_NOFOLLOW, DEFFILEMODE);
+  fd = __open64_nocancel (filename, O_RDWR | O_CREAT | O_NOFOLLOW
+			  | O_CLOEXEC, DEFFILEMODE);
   if (fd == -1)
     {
       char buf[400];
diff --git a/gmon/gmon.c b/gmon/gmon.c
index bc0e2943..6439ed1c 100644
--- a/gmon/gmon.c
+++ b/gmon/gmon.c
@@ -384,13 +384,14 @@ write_gmon (void)
 	size_t len = strlen (env);
 	char buf[len + 20];
 	__snprintf (buf, sizeof (buf), "%s.%u", env, __getpid ());
-	fd = __open_nocancel (buf, O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW, 0666);
+	fd = __open_nocancel (buf, O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW
+			      | O_CLOEXEC, 0666);
       }
 
     if (fd == -1)
       {
-	fd = __open_nocancel ("gmon.out", O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW,
-			      0666);
+	fd = __open_nocancel ("gmon.out", O_CREAT | O_TRUNC | O_WRONLY
+			      | O_NOFOLLOW | O_CLOEXEC, 0666);
 	if (fd < 0)
 	  {
 	    char buf[300];
diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index f2100ca8..87136e24 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -58,7 +58,7 @@ __gconv_load_cache (void)
     return -1;
 
   /* See whether the cache file exists.  */
-  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY, 0);
+  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY | O_CLOEXEC, 0);
   if (__builtin_expect (fd, 0) == -1)
     /* Not available.  */
     return -1;
diff --git a/login/utmp_file.c b/login/utmp_file.c
index 53494595..1ef07821 100644
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -463,7 +463,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
   int fd;
 
   /* Open WTMP file.  */
-  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE);
+  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE | O_CLOEXEC);
   if (fd < 0)
     return -1;
 
diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
index 2d32a135..e5db929d 100644
--- a/sysdeps/pthread/sem_open.c
+++ b/sysdeps/pthread/sem_open.c
@@ -36,6 +36,7 @@ sem_t *
 __sem_open (const char *name, int oflag, ...)
 {
   int fd;
+  int open_flags;
   sem_t *result;
 
   /* Check that shared futexes are supported.  */
@@ -64,9 +65,10 @@ __sem_open (const char *name, int oflag, ...)
   /* If the semaphore object has to exist simply open it.  */
   if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
     {
+      open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC;
+      open_flags |= (oflag & ~(O_CREAT|O_ACCMODE));
     try_again:
-      fd = __open (dirname.name,
-		   (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR);
+      fd = __open (dirname.name, open_flags);
 
       if (fd == -1)
 	{
@@ -133,7 +135,8 @@ __sem_open (const char *name, int oflag, ...)
 	    }
 
 	  /* Open the file.  Make sure we do not overwrite anything.  */
-	  fd = __open (tmpfname, O_RDWR | O_CREAT | O_EXCL, mode);
+	  open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC;
+	  fd = __open (tmpfname, open_flags, mode);
 	  if (fd == -1)
 	    {
 	      if (errno == EEXIST)
-- 
2.40.0

Re: [RFC PATCH v2 3/7] Use O_CLOEXEC in more places (BZ #15722)

[Adhemerval Zanella Netto]

On 19/04/23 13:02, Sergey Bugaev wrote:
> When opening a temporary file without O_CLOEXEC we risk leaking the
> file descriptor if another thread calls (fork and then) exec while we
> have the fd open. Fix this by consistently passing O_CLOEXEC everywhere
> where we open a file for internal use (and not to return it to the user,
> in which case the API defines whether or not the close-on-exec flag
> shall be set on the returned fd).
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

LGTM, thanks.  I can be installed independently.

> ---
>  catgets/open_catalog.c     | 4 ++--
>  elf/dl-profile.c           | 3 ++-
>  gmon/gmon.c                | 7 ++++---
>  iconv/gconv_cache.c        | 2 +-
>  login/utmp_file.c          | 2 +-
>  sysdeps/pthread/sem_open.c | 9 ++++++---
>  6 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
> index 242709db..46c444d2 100644
> --- a/catgets/open_catalog.c
> +++ b/catgets/open_catalog.c
> @@ -49,7 +49,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
>    char *buf = NULL;
>  
>    if (strchr (cat_name, '/') != NULL || nlspath == NULL)
> -    fd = __open_nocancel (cat_name, O_RDONLY);
> +    fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC);
>    else
>      {
>        const char *run_nlspath = nlspath;
> @@ -177,7 +177,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
>  
>  	  if (bufact != 0)
>  	    {
> -	      fd = __open_nocancel (buf, O_RDONLY);
> +	      fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC);
>  	      if (fd >= 0)
>  		break;
>  	    }
> diff --git a/elf/dl-profile.c b/elf/dl-profile.c
> index 2ecac05f..d8345da2 100644
> --- a/elf/dl-profile.c
> +++ b/elf/dl-profile.c
> @@ -324,7 +324,8 @@ _dl_start_profile (void)
>    *cp++ = '/';
>    __stpcpy (__stpcpy (cp, GLRO(dl_profile)), ".profile");
>  
> -  fd = __open64_nocancel (filename, O_RDWR|O_CREAT|O_NOFOLLOW, DEFFILEMODE);
> +  fd = __open64_nocancel (filename, O_RDWR | O_CREAT | O_NOFOLLOW
> +			  | O_CLOEXEC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        char buf[400];
> diff --git a/gmon/gmon.c b/gmon/gmon.c
> index bc0e2943..6439ed1c 100644
> --- a/gmon/gmon.c
> +++ b/gmon/gmon.c
> @@ -384,13 +384,14 @@ write_gmon (void)
>  	size_t len = strlen (env);
>  	char buf[len + 20];
>  	__snprintf (buf, sizeof (buf), "%s.%u", env, __getpid ());
> -	fd = __open_nocancel (buf, O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW, 0666);
> +	fd = __open_nocancel (buf, O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW
> +			      | O_CLOEXEC, 0666);
>        }
>  
>      if (fd == -1)
>        {
> -	fd = __open_nocancel ("gmon.out", O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW,
> -			      0666);
> +	fd = __open_nocancel ("gmon.out", O_CREAT | O_TRUNC | O_WRONLY
> +			      | O_NOFOLLOW | O_CLOEXEC, 0666);
>  	if (fd < 0)
>  	  {
>  	    char buf[300];
> diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
> index f2100ca8..87136e24 100644
> --- a/iconv/gconv_cache.c
> +++ b/iconv/gconv_cache.c
> @@ -58,7 +58,7 @@ __gconv_load_cache (void)
>      return -1;
>  
>    /* See whether the cache file exists.  */
> -  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY, 0);
> +  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY | O_CLOEXEC, 0);
>    if (__builtin_expect (fd, 0) == -1)
>      /* Not available.  */
>      return -1;
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 53494595..1ef07821 100644
> --- a/login/utmp_file.c
> +++ b/login/utmp_file.c
> @@ -463,7 +463,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
>    int fd;
>  
>    /* Open WTMP file.  */
> -  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE);
> +  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE | O_CLOEXEC);
>    if (fd < 0)
>      return -1;
>  
> diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
> index 2d32a135..e5db929d 100644
> --- a/sysdeps/pthread/sem_open.c
> +++ b/sysdeps/pthread/sem_open.c
> @@ -36,6 +36,7 @@ sem_t *
>  __sem_open (const char *name, int oflag, ...)
>  {
>    int fd;
> +  int open_flags;
>    sem_t *result;
>  
>    /* Check that shared futexes are supported.  */
> @@ -64,9 +65,10 @@ __sem_open (const char *name, int oflag, ...)
>    /* If the semaphore object has to exist simply open it.  */
>    if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
>      {
> +      open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC;
> +      open_flags |= (oflag & ~(O_CREAT|O_ACCMODE));
>      try_again:
> -      fd = __open (dirname.name,
> -		   (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR);
> +      fd = __open (dirname.name, open_flags);
>  
>        if (fd == -1)
>  	{
> @@ -133,7 +135,8 @@ __sem_open (const char *name, int oflag, ...)
>  	    }
>  
>  	  /* Open the file.  Make sure we do not overwrite anything.  */
> -	  fd = __open (tmpfname, O_RDWR | O_CREAT | O_EXCL, mode);
> +	  open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC;
> +	  fd = __open (tmpfname, open_flags, mode);
>  	  if (fd == -1)
>  	    {
>  	      if (errno == EEXIST)

Re: [RFC PATCH v2 3/7] Use O_CLOEXEC in more places (BZ #15722)

[Samuel Thibault]

Applied, thanks!

Adhemerval Zanella Netto, le ven. 21 avril 2023 09:55:58 -0300, a ecrit:
> 
> 
> On 19/04/23 13:02, Sergey Bugaev wrote:
> > When opening a temporary file without O_CLOEXEC we risk leaking the
> > file descriptor if another thread calls (fork and then) exec while we
> > have the fd open. Fix this by consistently passing O_CLOEXEC everywhere
> > where we open a file for internal use (and not to return it to the user,
> > in which case the API defines whether or not the close-on-exec flag
> > shall be set on the returned fd).
> > 
> > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> 
> LGTM, thanks.  I can be installed independently.
> 
> > ---
> >  catgets/open_catalog.c     | 4 ++--
> >  elf/dl-profile.c           | 3 ++-
> >  gmon/gmon.c                | 7 ++++---
> >  iconv/gconv_cache.c        | 2 +-
> >  login/utmp_file.c          | 2 +-
> >  sysdeps/pthread/sem_open.c | 9 ++++++---
> >  6 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
> > index 242709db..46c444d2 100644
> > --- a/catgets/open_catalog.c
> > +++ b/catgets/open_catalog.c
> > @@ -49,7 +49,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
> >    char *buf = NULL;
> >  
> >    if (strchr (cat_name, '/') != NULL || nlspath == NULL)
> > -    fd = __open_nocancel (cat_name, O_RDONLY);
> > +    fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC);
> >    else
> >      {
> >        const char *run_nlspath = nlspath;
> > @@ -177,7 +177,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
> >  
> >  	  if (bufact != 0)
> >  	    {
> > -	      fd = __open_nocancel (buf, O_RDONLY);
> > +	      fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC);
> >  	      if (fd >= 0)
> >  		break;
> >  	    }
> > diff --git a/elf/dl-profile.c b/elf/dl-profile.c
> > index 2ecac05f..d8345da2 100644
> > --- a/elf/dl-profile.c
> > +++ b/elf/dl-profile.c
> > @@ -324,7 +324,8 @@ _dl_start_profile (void)
> >    *cp++ = '/';
> >    __stpcpy (__stpcpy (cp, GLRO(dl_profile)), ".profile");
> >  
> > -  fd = __open64_nocancel (filename, O_RDWR|O_CREAT|O_NOFOLLOW, DEFFILEMODE);
> > +  fd = __open64_nocancel (filename, O_RDWR | O_CREAT | O_NOFOLLOW
> > +			  | O_CLOEXEC, DEFFILEMODE);
> >    if (fd == -1)
> >      {
> >        char buf[400];
> > diff --git a/gmon/gmon.c b/gmon/gmon.c
> > index bc0e2943..6439ed1c 100644
> > --- a/gmon/gmon.c
> > +++ b/gmon/gmon.c
> > @@ -384,13 +384,14 @@ write_gmon (void)
> >  	size_t len = strlen (env);
> >  	char buf[len + 20];
> >  	__snprintf (buf, sizeof (buf), "%s.%u", env, __getpid ());
> > -	fd = __open_nocancel (buf, O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW, 0666);
> > +	fd = __open_nocancel (buf, O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW
> > +			      | O_CLOEXEC, 0666);
> >        }
> >  
> >      if (fd == -1)
> >        {
> > -	fd = __open_nocancel ("gmon.out", O_CREAT|O_TRUNC|O_WRONLY|O_NOFOLLOW,
> > -			      0666);
> > +	fd = __open_nocancel ("gmon.out", O_CREAT | O_TRUNC | O_WRONLY
> > +			      | O_NOFOLLOW | O_CLOEXEC, 0666);
> >  	if (fd < 0)
> >  	  {
> >  	    char buf[300];
> > diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
> > index f2100ca8..87136e24 100644
> > --- a/iconv/gconv_cache.c
> > +++ b/iconv/gconv_cache.c
> > @@ -58,7 +58,7 @@ __gconv_load_cache (void)
> >      return -1;
> >  
> >    /* See whether the cache file exists.  */
> > -  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY, 0);
> > +  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY | O_CLOEXEC, 0);
> >    if (__builtin_expect (fd, 0) == -1)
> >      /* Not available.  */
> >      return -1;
> > diff --git a/login/utmp_file.c b/login/utmp_file.c
> > index 53494595..1ef07821 100644
> > --- a/login/utmp_file.c
> > +++ b/login/utmp_file.c
> > @@ -463,7 +463,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
> >    int fd;
> >  
> >    /* Open WTMP file.  */
> > -  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE);
> > +  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE | O_CLOEXEC);
> >    if (fd < 0)
> >      return -1;
> >  
> > diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
> > index 2d32a135..e5db929d 100644
> > --- a/sysdeps/pthread/sem_open.c
> > +++ b/sysdeps/pthread/sem_open.c
> > @@ -36,6 +36,7 @@ sem_t *
> >  __sem_open (const char *name, int oflag, ...)
> >  {
> >    int fd;
> > +  int open_flags;
> >    sem_t *result;
> >  
> >    /* Check that shared futexes are supported.  */
> > @@ -64,9 +65,10 @@ __sem_open (const char *name, int oflag, ...)
> >    /* If the semaphore object has to exist simply open it.  */
> >    if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
> >      {
> > +      open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC;
> > +      open_flags |= (oflag & ~(O_CREAT|O_ACCMODE));
> >      try_again:
> > -      fd = __open (dirname.name,
> > -		   (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR);
> > +      fd = __open (dirname.name, open_flags);
> >  
> >        if (fd == -1)
> >  	{
> > @@ -133,7 +135,8 @@ __sem_open (const char *name, int oflag, ...)
> >  	    }
> >  
> >  	  /* Open the file.  Make sure we do not overwrite anything.  */
> > -	  fd = __open (tmpfname, O_RDWR | O_CREAT | O_EXCL, mode);
> > +	  open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC;
> > +	  fd = __open (tmpfname, open_flags, mode);
> >  	  if (fd == -1)
> >  	    {
> >  	      if (errno == EEXIST)
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

[RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Sergey Bugaev]

stdin is supposed to be readable, stdout and stderr writable. Otherwise,
we get this:

l-wx------. 1 root root 64 Apr 19 18:40 0 -> /dev/full
lr-x------. 1 root root 64 Apr 19 18:40 1 -> /dev/null
lr-x------. 1 root root 64 Apr 19 18:40 2 -> /dev/null

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 csu/check_fds.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/csu/check_fds.c b/csu/check_fds.c
index de6dd716..ca4812cd 100644
--- a/csu/check_fds.c
+++ b/csu/check_fds.c
@@ -90,7 +90,7 @@ __libc_check_standard_fds (void)
      is really paranoid but some people actually are.  If /dev/null
      should happen to be a symlink to somewhere else and not the
      device commonly known as "/dev/null" we bail out.  */
-  check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW);
-  check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW);
-  check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW);
+  check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
+  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
+  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
 }
-- 
2.40.0

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Cristian Rodríguez]

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

On Wed, Apr 19, 2023 at 12:02 PM Sergey Bugaev <bugaevc@gmail.com> wrote:

> stdin is supposed to be readable, stdout and stderr writable. Otherwise,
> we get this:
>
> index de6dd716..ca4812cd 100644
> --- a/csu/check_fds.c
> +++ b/csu/check_fds.c
> @@ -90,7 +90,7 @@ __libc_check_standard_fds (void)
>       is really paranoid but some people actually are.  If /dev/null
>       should happen to be a symlink to somewhere else and not the
>       device commonly known as "/dev/null" we bail out.  */
> -  check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW);
> -  check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW);
> -  check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW);
> +  check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
> +  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
> +  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
>  }
> --
> 2.40.0
>

Im a little bit lost on what it was supposed to do in this old form..  as
the open flags are all wrong..

Changelog says:

 (__libc_check_standard_fds): Reverse modes so that common operations on
        the descriptors fail.

So this was intended at some point in the past decades to make it fail..  I
see it is used only for SUID statically linked binaries. is this really
needed now? playing silly games with this fds will always result in silly
prices.

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Sergey Bugaev]

On Wed, Apr 19, 2023, 22:13 Cristian Rodríguez <crrodriguez@opensuse.org> wrote:
> Im a little bit lost on what it was supposed to do in this old form..  as the open flags are all wrong..
>
> Changelog says:
>
>  (__libc_check_standard_fds): Reverse modes so that common operations on
>         the descriptors fail.
>
> So this was intended at some point in the past decades to make it fail..

Ah, I see, so I just failed to trace it through git blame / git log,
because the commit renaming this file sysdeps/generic/check_fds.c ->
csu/check_fds.c has been done wrong (or maybe this is an artefact of
the migration to Git).

So which way would you prefer it, left as-is (i.e. without this patch)
or switched back? If we leave it as is, we should at least add a
comment explaining what's going on, for the next person who stumbles
into this and also fails to trace it through git blame.

> I see it is used only for SUID statically linked binaries.

I might be missing something, but why statically linked only? I don't
see anything like that in elf/Makefile (but maybe I don't know where
to look, please tell me!), and also the same behavior is certainly
exhibited by dynamically linked executables too. That ls -l I posted
above is from a dynamic executable.

> is this really needed now? playing silly games with this fds will always result in silly prices.

Sergey

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Adhemerval Zanella Netto]

On 19/04/23 16:40, Sergey Bugaev wrote:
> On Wed, Apr 19, 2023, 22:13 Cristian Rodríguez <crrodriguez@opensuse.org> wrote:
>> Im a little bit lost on what it was supposed to do in this old form..  as the open flags are all wrong..
>>
>> Changelog says:
>>
>>  (__libc_check_standard_fds): Reverse modes so that common operations on
>>         the descriptors fail.
>>
>> So this was intended at some point in the past decades to make it fail..
> 
> Ah, I see, so I just failed to trace it through git blame / git log,
> because the commit renaming this file sysdeps/generic/check_fds.c ->
> csu/check_fds.c has been done wrong (or maybe this is an artefact of
> the migration to Git).
> 
> So which way would you prefer it, left as-is (i.e. without this patch)
> or switched back? If we leave it as is, we should at least add a
> comment explaining what's going on, for the next person who stumbles
> into this and also fails to trace it through git blame.
> 
>> I see it is used only for SUID statically linked binaries.
> 
> I might be missing something, but why statically linked only? I don't
> see anything like that in elf/Makefile (but maybe I don't know where
> to look, please tell me!), and also the same behavior is certainly
> exhibited by dynamically linked executables too. That ls -l I posted
> above is from a dynamic executable.

At least on Hurd, __libc_check_standard_fds is only built for !SHARED.

> 
>> is this really needed now? playing silly games with this fds will always result in silly prices.

My understanding of this code is to enforce that on setuid program with
stdin/stdout/stderr closed any operation fail.  The original commit
db33f7d4aef7 that added these had this specific comment:

  /* Protec SUID program against misuse of file descriptors.  */
  extern void __libc_check_standard_fds (void);

Maybe we can add extend the comment on this file to add the intention
of this code.

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Sergey Bugaev]

On Wed, Apr 19, 2023 at 11:45 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> > I might be missing something, but why statically linked only? I don't
> > see anything like that in elf/Makefile (but maybe I don't know where
> > to look, please tell me!), and also the same behavior is certainly
> > exhibited by dynamically linked executables too. That ls -l I posted
> > above is from a dynamic executable.
>
> At least on Hurd, __libc_check_standard_fds is only built for !SHARED.

Right, so I see that there is a separate Hurd version of this. My
patch attempted to change the generic csu/check_fds.c, not the Hurd
version. (Maybe there's no need to add O_IGNORE_CTTY into the generic
version then).

In the Hurd version all the logic is in init_standard_fds (which runs
through the _hurd_fd_subinit hook) and not in
__libc_check_standard_fds (), which is empty and indeed only built
for !SHARED -- as the comment there says, to make sure this file is
pulled in when linking against libc.a.

Also the Hurd version still does

  check_one_fd (STDIN_FILENO, O_RDONLY);
  check_one_fd (STDOUT_FILENO, O_RDWR);
  check_one_fd (STDERR_FILENO, O_RDWR);

and opens /dev/null (not /dev/full), so in any case either the
generic one needs changes, or the Hurd one. If we want to port the
"always fail" change onto the Hurd version, we can take advantage of
the fact that on the Hurd we can truly open /dev/null with mode = 0
(not O_RDONLY), and it will be neither readable nor writable.

> >> is this really needed now? playing silly games with this fds will always result in silly prices.
>
> My understanding of this code is to enforce that on setuid program with
> stdin/stdout/stderr closed any operation fail.

Yes, but is that still considered desirable / a good idea? As opposed
to making such operations no-op successfully (opening /dev/null with
the expected mode).

Sergey

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Adhemerval Zanella Netto]

On 19/04/23 18:16, Sergey Bugaev wrote:
> On Wed, Apr 19, 2023 at 11:45 PM Adhemerval Zanella Netto
>>>> is this really needed now? playing silly games with this fds will always result in silly prices.
>>
>> My understanding of this code is to enforce that on setuid program with
>> stdin/stdout/stderr closed any operation fail.
> 
> Yes, but is that still considered desirable / a good idea? As opposed
> to making such operations no-op successfully (opening /dev/null with
> the expected mode).
> 

Good question, this is essentially a hardening for setsuid binaries since
opening the file in the *expected* way is not the intended behavior (even
though the C runtime expects that STDIN_FILENO, STDOUT_FILENO, and
STDERR_FILENO are in fact opened). As far I could check, this is really a 
glibc extension (both FreeBSD and OpenBSD does not seem to add such 
hardening).

I am not really sure how effective is this hardening, it seems more a
development one to enforce that system daemon are spawned correctly.

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Cristian Rodríguez]

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

On Thu, Apr 20, 2023 at 7:47 AM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

>
>
>
> I am not really sure how effective is this hardening, it seems more a
> development one to enforce that system daemon are spawned correctly.
>

Exactly, my understanding is that it is a futile exercise ..if one
sufficient privilege at that stage one can do whatever is desired..  why
even bother messing with the standard fds..

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Adhemerval Zanella Netto]

On 20/04/23 09:06, Cristian Rodríguez wrote:
> 
> 
> On Thu, Apr 20, 2023 at 7:47 AM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
> 
> 
> 
> 
>     I am not really sure how effective is this hardening, it seems more a
>     development one to enforce that system daemon are spawned correctly.
> 
> 
> Exactly, my understanding is that it is a futile exercise ..if one sufficient privilege at that stage one can do whatever is desired..  why even bother messing with the standard fds..

I don't have a strong opinion, but I tend to agree that this hardening does
not add much specially now that we have a lot of granular ways to limit 
process execution (such as capabilities, seccomp, etc.).

Re: [RFC PATCH v2 4/7] csu: Fix standard fds' mode

[Paul Eggert]

On 2023-04-20 05:06, Cristian Rodríguez via Libc-alpha wrote:
> Exactly, my understanding is that it is a futile exercise ..if one
> sufficient privilege at that stage one can do whatever is desired..  why
> even bother messing with the standard fds..

Making stdin unreadable is not meant to thwart a root-privileged 
attacker. As the comment in check_one_fd says, it's merely meant to 
catch bugs in programs that accidentally (for example) read from 
standard input even though there is no standard input. If standard input 
is /dev/null and readable, these buggy programs silently behave as if 
the input is the empty file, which is likely incorrect. In contrast, if 
standard input is not readable, these buggy programs will get a read 
error, which is more likely to cause them to report an error and alert 
users of the bug.

So let's leave glibc/csu/check_fds.c alone: it serves a useful purpose.

PS. Sorry if this email is duplicate; I had mail server problems.

[RFC PATCH v2 5/7] hurd: Make dl-sysdep's open () cope with O_IGNORE_CTTY

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/dl-sysdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
index 2d595d00..6e167e12 100644
--- a/sysdeps/mach/hurd/dl-sysdep.c
+++ b/sysdeps/mach/hurd/dl-sysdep.c
@@ -289,8 +289,8 @@ open_file (const char *file_name, int flags,
       return MACH_PORT_NULL;
     }
 
-  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC)));
-  flags &= ~O_CLOEXEC;
+  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC | O_IGNORE_CTTY)));
+  flags &= ~(O_CLOEXEC | O_IGNORE_CTTY);
 
   startdir = _dl_hurd_data->portarray[file_name[0] == '/'
 				      ? INIT_PORT_CRDIR : INIT_PORT_CWDIR];
-- 
2.40.0

Re: [RFC PATCH v2 5/7] hurd: Make dl-sysdep's open () cope with O_IGNORE_CTTY

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 19 avril 2023 19:02:05 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/dl-sysdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 2d595d00..6e167e12 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -289,8 +289,8 @@ open_file (const char *file_name, int flags,
>        return MACH_PORT_NULL;
>      }
>  
> -  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC)));
> -  flags &= ~O_CLOEXEC;
> +  assert (!(flags & ~(O_READ | O_EXEC | O_CLOEXEC | O_IGNORE_CTTY)));
> +  flags &= ~(O_CLOEXEC | O_IGNORE_CTTY);
>  
>    startdir = _dl_hurd_data->portarray[file_name[0] == '/'
>  				      ? INIT_PORT_CRDIR : INIT_PORT_CWDIR];
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

[RFC PATCH v2 6/7] include/fcntl.h: Define O_IGNORE_CTTY

[Sergey Bugaev]

This internal definition makes it possible to use O_IGNORE_CTTY in
the glibc codebase unconditionally, no matter whether the current port
provides it or not (i.e. both on Hurd and on Linux). Along with the
definition, this adds a small guide on when O_IGNORE_CTTY is to be used.

The following commit will actually make use of O_IGNORE_CTTY
throughout the glibc codebase.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 include/fcntl.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/fcntl.h b/include/fcntl.h
index be435047..d788db2e 100644
--- a/include/fcntl.h
+++ b/include/fcntl.h
@@ -33,6 +33,21 @@ extern int __openat_2 (int __fd, const char *__path, int __oflag);
 extern int __openat64_2 (int __fd, const char *__path, int __oflag);
 
 
+/* Makes open () & friends faster on the Hurd, but can only be used (without
+   altering user-visible behavior) when we're sure that the file we're opening
+   is not (at the moment) our controlling terminal.  Use this when:
+   - opening well-known files internally (utmp, nss db);
+   - opening files with user-specified names that can not reasonably be ttys
+     (sem_open, shm_open);
+   - opening new (previously unused) ttys (openpty).
+   Don't use this when:
+   - doing a general-purpose open () with a user-controlled path that could
+     well be "/dev/tty" (fopen).  */
+#ifndef O_IGNORE_CTTY
+#  define O_IGNORE_CTTY	0
+#endif
+
+
 #if IS_IN (rtld)
 #  include <dl-fcntl.h>
 #endif
-- 
2.40.0

[RFC PATCH v2 7/7] Use O_IGNORE_CTTY where appropriate

[Sergey Bugaev]

* getpt, openpty: Opening an unused pty, which can't be our ctty
* shm_open, sem_open: These don't work with ttys
* opendir: Directories are unlikely to be ttys

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 catgets/open_catalog.c      | 4 ++--
 csu/check_fds.c             | 6 +++---
 elf/dl-load.c               | 2 +-
 elf/dl-misc.c               | 2 +-
 elf/dl-profile.c            | 2 +-
 gmon/gmon.c                 | 4 ++--
 iconv/gconv_cache.c         | 3 ++-
 locale/loadarchive.c        | 7 ++++---
 locale/loadlocale.c         | 4 ++--
 login/openpty.c             | 2 +-
 login/utmp_file.c           | 7 ++++---
 misc/daemon.c               | 2 +-
 nss/nss_db/db-open.c        | 3 ++-
 rt/shm_open.c               | 2 +-
 shadow/lckpwdf.c            | 2 +-
 sysdeps/mach/hurd/opendir.c | 2 +-
 sysdeps/pthread/sem_open.c  | 4 ++--
 sysdeps/unix/bsd/getpt.c    | 4 ++--
 18 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
index 46c444d2..129f4662 100644
--- a/catgets/open_catalog.c
+++ b/catgets/open_catalog.c
@@ -49,7 +49,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
   char *buf = NULL;
 
   if (strchr (cat_name, '/') != NULL || nlspath == NULL)
-    fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC);
+    fd = __open_nocancel (cat_name, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
   else
     {
       const char *run_nlspath = nlspath;
@@ -177,7 +177,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
 
 	  if (bufact != 0)
 	    {
-	      fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC);
+	      fd = __open_nocancel (buf, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
 	      if (fd >= 0)
 		break;
 	    }
diff --git a/csu/check_fds.c b/csu/check_fds.c
index ca4812cd..59565f92 100644
--- a/csu/check_fds.c
+++ b/csu/check_fds.c
@@ -90,7 +90,7 @@ __libc_check_standard_fds (void)
      is really paranoid but some people actually are.  If /dev/null
      should happen to be a symlink to somewhere else and not the
      device commonly known as "/dev/null" we bail out.  */
-  check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
-  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
-  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
+  check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW | O_IGNORE_CTTY);
+  check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW | O_IGNORE_CTTY);
+  check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW | O_IGNORE_CTTY);
 }
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9a0e40c0..009484f1 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1616,7 +1616,7 @@ open_verify (const char *name, int fd,
 
   if (fd == -1)
     /* Open the file.  We always open files read-only.  */
-    fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC);
+    fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
 
   if (fd != -1)
     {
diff --git a/elf/dl-misc.c b/elf/dl-misc.c
index 5b84adc2..85931c7c 100644
--- a/elf/dl-misc.c
+++ b/elf/dl-misc.c
@@ -36,7 +36,7 @@ _dl_sysdep_read_whole_file (const char *file, size_t *sizep, int prot)
 {
   void *result = MAP_FAILED;
   struct __stat64_t64 st;
-  int fd = __open64_nocancel (file, O_RDONLY | O_CLOEXEC);
+  int fd = __open64_nocancel (file, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
   if (fd >= 0)
     {
       if (__fstat64_time64 (fd, &st) >= 0)
diff --git a/elf/dl-profile.c b/elf/dl-profile.c
index d8345da2..ff97b129 100644
--- a/elf/dl-profile.c
+++ b/elf/dl-profile.c
@@ -325,7 +325,7 @@ _dl_start_profile (void)
   __stpcpy (__stpcpy (cp, GLRO(dl_profile)), ".profile");
 
   fd = __open64_nocancel (filename, O_RDWR | O_CREAT | O_NOFOLLOW
-			  | O_CLOEXEC, DEFFILEMODE);
+			  | O_CLOEXEC | O_IGNORE_CTTY, DEFFILEMODE);
   if (fd == -1)
     {
       char buf[400];
diff --git a/gmon/gmon.c b/gmon/gmon.c
index 6439ed1c..ed13b3ce 100644
--- a/gmon/gmon.c
+++ b/gmon/gmon.c
@@ -385,13 +385,13 @@ write_gmon (void)
 	char buf[len + 20];
 	__snprintf (buf, sizeof (buf), "%s.%u", env, __getpid ());
 	fd = __open_nocancel (buf, O_CREAT | O_TRUNC | O_WRONLY | O_NOFOLLOW
-			      | O_CLOEXEC, 0666);
+			      | O_CLOEXEC | O_IGNORE_CTTY, 0666);
       }
 
     if (fd == -1)
       {
 	fd = __open_nocancel ("gmon.out", O_CREAT | O_TRUNC | O_WRONLY
-			      | O_NOFOLLOW | O_CLOEXEC, 0666);
+			      | O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY, 0666);
 	if (fd < 0)
 	  {
 	    char buf[300];
diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index 87136e24..c8d972c8 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -58,7 +58,8 @@ __gconv_load_cache (void)
     return -1;
 
   /* See whether the cache file exists.  */
-  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY | O_CLOEXEC, 0);
+  fd = __open_nocancel (GCONV_MODULES_CACHE, O_RDONLY
+			| O_CLOEXEC | O_IGNORE_CTTY, 0);
   if (__builtin_expect (fd, 0) == -1)
     /* Not available.  */
     return -1;
diff --git a/locale/loadarchive.c b/locale/loadarchive.c
index 5b857d5d..f88ff8b8 100644
--- a/locale/loadarchive.c
+++ b/locale/loadarchive.c
@@ -202,7 +202,8 @@ _nl_load_locale_from_archive (int category, const char **namep)
       archmapped = &headmap;
 
       /* The archive has never been opened.  */
-      fd = __open_nocancel (archfname, O_RDONLY|O_LARGEFILE|O_CLOEXEC);
+      fd = __open_nocancel (archfname, O_RDONLY | O_LARGEFILE
+			    | O_CLOEXEC | O_IGNORE_CTTY);
       if (fd < 0)
 	/* Cannot open the archive, for whatever reason.  */
 	return NULL;
@@ -397,8 +398,8 @@ _nl_load_locale_from_archive (int category, const char **namep)
 	  if (fd == -1)
 	    {
 	      struct __stat64_t64 st;
-	      fd = __open_nocancel (archfname,
-				    O_RDONLY|O_LARGEFILE|O_CLOEXEC);
+	      fd = __open_nocancel (archfname, O_RDONLY | O_LARGEFILE
+				    | O_CLOEXEC | O_IGNORE_CTTY);
 	      if (fd == -1)
 		/* Cannot open the archive, for whatever reason.  */
 		return NULL;
diff --git a/locale/loadlocale.c b/locale/loadlocale.c
index 671e71cf..582144ed 100644
--- a/locale/loadlocale.c
+++ b/locale/loadlocale.c
@@ -240,7 +240,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category)
   file->decided = 1;
   file->data = NULL;
 
-  fd = __open_nocancel (file->filename, O_RDONLY | O_CLOEXEC);
+  fd = __open_nocancel (file->filename, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
   if (__builtin_expect (fd, 0) < 0)
     /* Cannot open the file.  */
     return;
@@ -267,7 +267,7 @@ _nl_load_locale (struct loaded_l10nfile *file, int category)
 			    "/SYS_", 5), _nl_category_names_get (category),
 		 _nl_category_name_sizes[category] + 1);
 
-      fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC);
+      fd = __open_nocancel (newp, O_RDONLY | O_CLOEXEC | O_IGNORE_CTTY);
       if (__builtin_expect (fd, 0) < 0)
 	return;
 
diff --git a/login/openpty.c b/login/openpty.c
index 1e44852a..a89555b2 100644
--- a/login/openpty.c
+++ b/login/openpty.c
@@ -117,7 +117,7 @@ __openpty (int *pptmx, int *pterminal, char *name,
       if (pts_name (ptmx, &buf, sizeof (_buf)))
         goto on_error;
 
-      terminal = __open64 (buf, O_RDWR | O_NOCTTY);
+      terminal = __open64 (buf, O_RDWR | O_NOCTTY | O_IGNORE_CTTY);
       if (terminal == -1)
         goto on_error;
     }
diff --git a/login/utmp_file.c b/login/utmp_file.c
index 1ef07821..a7815096 100644
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -142,7 +142,7 @@ __libc_setutent (void)
 
       file_writable = false;
       file_fd = __open_nocancel
-	(file_name, O_RDONLY | O_LARGEFILE | O_CLOEXEC);
+	(file_name, O_RDONLY | O_LARGEFILE | O_CLOEXEC | O_IGNORE_CTTY);
       if (file_fd == -1)
 	return 0;
     }
@@ -354,7 +354,7 @@ __libc_pututline (const struct utmp *data)
       const char *file_name = TRANSFORM_UTMP_FILE_NAME (__libc_utmp_file_name);
 
       int new_fd = __open_nocancel
-	(file_name, O_RDWR | O_LARGEFILE | O_CLOEXEC);
+	(file_name, O_RDWR | O_LARGEFILE | O_CLOEXEC | O_IGNORE_CTTY);
       if (new_fd == -1)
 	return NULL;
 
@@ -463,7 +463,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
   int fd;
 
   /* Open WTMP file.  */
-  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE | O_CLOEXEC);
+  fd = __open_nocancel (file, O_WRONLY | O_LARGEFILE
+			| O_CLOEXEC | O_IGNORE_CTTY);
   if (fd < 0)
     return -1;
 
diff --git a/misc/daemon.c b/misc/daemon.c
index 58dde4f0..60f11828 100644
--- a/misc/daemon.c
+++ b/misc/daemon.c
@@ -82,7 +82,7 @@ daemon (int nochdir, int noclose)
     {
       struct __stat64_t64 st;
 
-      fd = __open_nocancel (_PATH_DEVNULL, O_RDWR, 0);
+      fd = __open_nocancel (_PATH_DEVNULL, O_RDWR | O_IGNORE_CTTY, 0);
       if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0))
         {
           if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0
diff --git a/nss/nss_db/db-open.c b/nss/nss_db/db-open.c
index 13670600..c572d62c 100644
--- a/nss/nss_db/db-open.c
+++ b/nss/nss_db/db-open.c
@@ -36,7 +36,8 @@ internal_setent (const char *file, struct nss_db_map *mapping)
 {
   enum nss_status status = NSS_STATUS_UNAVAIL;
 
-  int fd = __open_nocancel (file, O_RDONLY | O_LARGEFILE | O_CLOEXEC);
+  int fd = __open_nocancel (file, O_RDONLY | O_LARGEFILE
+			    | O_CLOEXEC | O_IGNORE_CTTY);
   if (fd != -1)
     {
       struct nss_db_header header;
diff --git a/rt/shm_open.c b/rt/shm_open.c
index fc1dc96b..7fd62cf3 100644
--- a/rt/shm_open.c
+++ b/rt/shm_open.c
@@ -37,7 +37,7 @@ __shm_open (const char *name, int oflag, mode_t mode)
       return -1;
     }
 
-  oflag |= O_NOFOLLOW | O_CLOEXEC;
+  oflag |= O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY;
 #if defined (SHM_ANON) && defined (O_TMPFILE)
   if (name == SHM_ANON)
     oflag |= O_TMPFILE;
diff --git a/shadow/lckpwdf.c b/shadow/lckpwdf.c
index 3b36b2eb..4a623c41 100644
--- a/shadow/lckpwdf.c
+++ b/shadow/lckpwdf.c
@@ -96,7 +96,7 @@ __lckpwdf (void)
   /* Prevent problems caused by multiple threads.  */
   __libc_lock_lock (lock);
 
-  int oflags = O_WRONLY | O_CREAT | O_CLOEXEC;
+  int oflags = O_WRONLY | O_CREAT | O_CLOEXEC | O_IGNORE_CTTY;
   lock_fd = __open (PWD_LOCKFILE, oflags, 0600);
   if (lock_fd == -1)
     /* Cannot create lock file.  */
diff --git a/sysdeps/mach/hurd/opendir.c b/sysdeps/mach/hurd/opendir.c
index cfba659c..a9e8f94d 100644
--- a/sysdeps/mach/hurd/opendir.c
+++ b/sysdeps/mach/hurd/opendir.c
@@ -79,7 +79,7 @@ __opendirat (int dfd, const char *name)
       return NULL;
     }
 
-  int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC;
+  int flags = O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_IGNORE_CTTY;
   int fd;
 #if IS_IN (rtld)
   assert (dfd == AT_FDCWD);
diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
index e5db929d..5a248ebb 100644
--- a/sysdeps/pthread/sem_open.c
+++ b/sysdeps/pthread/sem_open.c
@@ -65,7 +65,7 @@ __sem_open (const char *name, int oflag, ...)
   /* If the semaphore object has to exist simply open it.  */
   if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
     {
-      open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC;
+      open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC | O_IGNORE_CTTY;
       open_flags |= (oflag & ~(O_CREAT|O_ACCMODE));
     try_again:
       fd = __open (dirname.name, open_flags);
@@ -135,7 +135,7 @@ __sem_open (const char *name, int oflag, ...)
 	    }
 
 	  /* Open the file.  Make sure we do not overwrite anything.  */
-	  open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC;
+	  open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_IGNORE_CTTY;
 	  fd = __open (tmpfname, open_flags, mode);
 	  if (fd == -1)
 	    {
diff --git a/sysdeps/unix/bsd/getpt.c b/sysdeps/unix/bsd/getpt.c
index 8369f958..48f3d07a 100644
--- a/sysdeps/unix/bsd/getpt.c
+++ b/sysdeps/unix/bsd/getpt.c
@@ -76,7 +76,7 @@ __bsd_openpt (int oflag)
 int
 __getpt (void)
 {
-  return __bsd_openpt (O_RDWR);
+  return __bsd_openpt (O_RDWR | O_IGNORE_CTTY);
 }
 libc_hidden_def (__getpt)
 weak_alias (__getpt, getpt)
@@ -84,6 +84,6 @@ weak_alias (__getpt, getpt)
 int
 __posix_openpt (int oflag)
 {
-  return __bsd_openpt (oflag);
+  return __bsd_openpt (oflag | O_IGNORE_CTTY);
 }
 weak_alias (__posix_openpt, posix_openpt)
-- 
2.40.0