[RFC PATCH 0/5] O_IGNORE_CTTY everywhere

[RFC PATCH 0/5] O_IGNORE_CTTY everywhere

[Sergey Bugaev]

Hello!

This is an attempt to use O_IGNORE_CTTY in a lot of places throughout
glibc. But first, what is O_IGNORE_CTTY, how is it different from
O_NOCTTY, and why would we want to use it?

O_IGNORE_CTTY is an open () flag in the Hurd port that makes glibc
access the newly opened file as if it's not the controlling terminal
of the process, even if it actually really is. In particular, reads
from the file while in the background won't generate SIGTTIN, writes
won't generate SIGTTOU. (Doesn't sound very useful yet, does it?)

O_NOCTTY, in contrast, controls whether or not the newly opened file,
if it is a terminal, automatically becomes the ctty of the process. On
the Hurd this is never the case, and O_NOCTTY is defined to 0.

So, why would one want to use O_IGNORE_CTTY when opening random files
that have nothing to do with terminals, let alone cttys? Well, it
turns out you *especially* want to use O_IGNORE_CTTY precisely in case
you know that what you're opening surely is not the current ctty of
the process! This is because unless O_IGNORE_CTTY is set, glibc has to
do an extra RPC (or even two) to check whether the file is our ctty.
Passing O_IGNORE_CTTY is a hint/promise to glibc that the file is not
our ctty, so theres' no need to make the expensive check.

So, O_IGNORE_CTTY is most useful not to alter the user-visible
behavior (treat ctty as if it's not that), but rather to speed up
open () calls *without* altering user-visible behavior.

=======================================

With that in mind, I went through the codebase and added O_IGNORE_CTTY
to __open flags just about everywhere. You only want to *not* pass
O_IGNORE_CTTY when you're not sure what you're opening, such as when
implementing fopen () or a similar wrapper for open (). But when
you're opening, say, a shared library, or a /dev/shm/ file, or a
locale archive -- you know you're not dealing with ttys, and so using
O_IGNORE_CTTY is appropriate and beneficial.

In order to not add tons of ifdefs, I've added a single

#ifndef O_IGNORE_CTTY
#  define O_IGNORE_CTTY	0
#endif

to include/fcntl.h (following Samuel's advice). This of course makes
O_IGNORE_CTTY usable (and no-op) on Linux (and other ports) without
exposing it in the installed headers.

=======================================

While looking through __open calls, I've noticed a few that
(suprisingly!) did not use O_CLOEXEC. I've tried to fix them all. Even
if the rest of this patchset is rejected, please evaluate that one
commit separately.

I had to convert misc/daemon.c to a more GNU-ish style, since my
changes wouldn't easily fit on 79/80 columns otherwise. I've split off
the actual conversion into a separate patch to make review easier; but
that patch is still going to unreadable; nothing I can do about that,
sorry.

I have built glibc on x86_64-linux and run the testsuite; there are no
new test failures. I can't run the full testsuite on i686-gnu, but
some manual testing confirms that the many superfluous term_getctty ()
RPC calls are gone, but the ctty functionality is still there, e.g. I
can fopen ("/dev/tty") and write to it and get the expected behavior.

This is different from my usual patches in that it mostly touches non-
Hurd-specific code.

Sergey

Sergey Bugaev (5):
  misc: Convert daemon () to GNU coding style
  Use O_CLOEXEC in more places
  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               | 16 +++++++
 locale/loadarchive.c          |  7 +--
 locale/loadlocale.c           |  4 +-
 login/openpty.c               |  2 +-
 login/utmp_file.c             |  7 +--
 misc/daemon.c                 | 88 +++++++++++++++++++----------------
 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, 106 insertions(+), 71 deletions(-)

-- 
2.39.2

[RFC PATCH 1/5] 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..61da49b7 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.39.2

[RFC PATCH 2/5] Use O_CLOEXEC in more places

[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).

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.39.2

[RFC PATCH 3/5] 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.39.2

[RFC PATCH 4/5] 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/fcntl.h b/include/fcntl.h
index be435047..3de40827 100644
--- a/include/fcntl.h
+++ b/include/fcntl.h
@@ -33,6 +33,22 @@ 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 slightly 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.39.2

[RFC PATCH 5/5] 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 de6dd716..636bc431 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_WRONLY | O_NOFOLLOW | O_IGNORE_CTTY);
+  check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW | O_IGNORE_CTTY);
+  check_one_fd (STDERR_FILENO, O_RDONLY | 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 61da49b7..d5bf173f 100644
--- a/misc/daemon.c
+++ b/misc/daemon.c
@@ -67,7 +67,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.39.2

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

[Adhemerval Zanella Netto]

On 17/04/23 19:58, Sergey Bugaev via Libc-alpha 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, some minor nits below.

> ---
>  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/misc/daemon.c b/misc/daemon.c
> index 3c73ac2a..61da49b7 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 ("/");

I think there is no need to ignore return code.

> +
> +  if (!noclose)
> +    {
> +      struct __stat64_t64 st;
> +
> +      fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0);

Missing space before '('.

> +      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 2/5] Use O_CLOEXEC in more places

[Adhemerval Zanella Netto]

On 17/04/23 19:58, Sergey Bugaev via Libc-alpha 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).
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

This is BZ#15722 [1], so I think you should advertise on subject. Patch
LGMT, thanks.

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

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

> ---
>  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 1/5] misc: Convert daemon () to GNU coding style

[Cristian Rodríguez]

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

On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha <
libc-alpha@sourceware.org> wrote:

>
>
> On 17/04/23 19:58, Sergey Bugaev via Libc-alpha 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, some minor nits below.
>
> > ---
> >  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 49 insertions(+), 39 deletions(-)
> >
> > diff --git a/misc/daemon.c b/misc/daemon.c
> > index 3c73ac2a..61da49b7 100644
> > --- a/misc/daemon.c
> > +++ b/misc/daemon.c
> > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c      8.1
> (Berkeley) 6/4/93";
>
>
> I think there is no need to ignore return code.
>
>
> Also This code clearly comes from freeBSD.. which has since updated the
code to ignore SIGHUP  when the parent exits.

https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c

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

[Adhemerval Zanella Netto]

On 18/04/23 10:48, Cristian Rodríguez wrote:
> 
> 
> On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org <mailto:libc-alpha@sourceware.org>> wrote:
> 
> 
> 
>     On 17/04/23 19:58, Sergey Bugaev via Libc-alpha 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 <mailto:bugaevc@gmail.com>>
> 
>     LGTM, some minor nits below.
> 
>     > ---
>     >  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
>     >  1 file changed, 49 insertions(+), 39 deletions(-)
>     >
>     > diff --git a/misc/daemon.c b/misc/daemon.c
>     > index 3c73ac2a..61da49b7 100644
>     > --- a/misc/daemon.c
>     > +++ b/misc/daemon.c
>     > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c      8.1 (Berkeley) 6/4/93";
> 
> 
>     I think there is no need to ignore return code.
> 
> 
> Also This code clearly comes from freeBSD.. which has since updated the code to ignore SIGHUP  when the parent exits.
> 
> https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c <https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c>

I think such change should be in a different patch though.

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

[Sergey Bugaev]

On Tue, Apr 18, 2023 at 9:49 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 18/04/23 10:48, Cristian Rodríguez wrote:
> >
> >
> > On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org <mailto:libc-alpha@sourceware.org>> wrote:
> >
> >
> >
> >     On 17/04/23 19:58, Sergey Bugaev via Libc-alpha 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 <mailto:bugaevc@gmail.com>>
> >
> >     LGTM, some minor nits below.
> >
> >     > ---
> >     >  misc/daemon.c | 88 ++++++++++++++++++++++++++++-----------------------
> >     >  1 file changed, 49 insertions(+), 39 deletions(-)
> >     >
> >     > diff --git a/misc/daemon.c b/misc/daemon.c
> >     > index 3c73ac2a..61da49b7 100644
> >     > --- a/misc/daemon.c
> >     > +++ b/misc/daemon.c
> >     > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c      8.1 (Berkeley) 6/4/93";
> >
> >
> >     I think there is no need to ignore return code.
> >
> >
> > Also This code clearly comes from freeBSD.. which has since updated the code to ignore SIGHUP  when the parent exits.
> >
> > https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c <https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c>
>
> I think such change should be in a different patch though.

I'm going to send out a v2 of this series with the proposed changes to
daemon () included (in a separate patch from reformatting it).

It is my understanding, though, that setting and resetting the signal
handler like that is racy, since signal delivery is asynchronous. We
may get a SIGHUP some time after daemon () completes, even.

Sergey