[PATCH v3 3/4] io: Add closefrom [BZ #10353]

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/io/Versions b/io/Versions
> index 49c4d2d40a..fa33452881 100644
> --- a/io/Versions
> +++ b/io/Versions
> @@ -135,6 +135,7 @@ libc {
>    GLIBC_2.33 {
>      stat; stat64; fstat; fstat64; lstat; lstat64; fstatat; fstatat64;
>      mknod; mknodat;
> +    closefrom;
>    }

Needs to be GLIBC_2.34 now.  Copyright years need adjusting, too.

> diff --git a/io/closefrom.c b/io/closefrom.c
> new file mode 100644
> index 0000000000..7833935b16
> --- /dev/null
> +++ b/io/closefrom.c

> +void
> +__closefrom (int lowfd)
> +{
> +  int maxfd = __getdtablesize ();
> +  if (maxfd == -1)
> +    __fortify_fail ("closefrom failed to get the file descriptor table size");
> +
> +  for (int i = 0; i < maxfd; i++)
> +    if (i >= lowfd)
> +      __close_nocancel_nostatus (i);
> +}

Still okay for Hurd.

> +weak_alias (__closefrom, closefrom)
> diff --git a/io/tst-closefrom.c b/io/tst-closefrom.c
> new file mode 100644
> index 0000000000..6f6fbd270f
> --- /dev/null
> +++ b/io/tst-closefrom.c

> +#define NFDS 100
> +
> +static int
> +open_multiple_temp_files (void)
> +{
> +  /* Check if the temporary file descriptor has no no gaps.  */
> +  int lowfd = xopen ("/dev/null", O_RDONLY, 0600);
> +  for (int i = 1; i <= NFDS; i++)
> +    TEST_COMPARE (xopen ("/dev/null", O_RDONLY, 0600),
> +		  lowfd + i);
> +  return lowfd;
> +}

Spurious line wrap.

> +
> +static int
> +closefrom_test (void)
> +{
> +  struct support_descriptors *descrs = support_descriptors_list ();
> +
> +  int lowfd = open_multiple_temp_files ();
> +
> +  const int maximum_fd = lowfd + NFDS;
> +  const int half_fd = maximum_fd / 2;
> +  const int gap = maximum_fd / 4;

See the other message about the half_fd initialization.

> +/* Check if closefrom works even when no new file descriptors can be
> +   created.  */
> +static int
> +closefrom_test_file_desc_limit (void)
> +{
> +  int max_fd = NFDS;
> +  {
> +    struct rlimit rl;
> +    if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
> +      FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
> +
> +    max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd);
> +    rl.rlim_cur = max_fd;
> +
> +    if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
> +      FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
> +  }
> +
> +  /* Exhauste the file descriptor limit.  */
> +  int lowfd = xopen ("/dev/null", O_RDONLY, 0600);
> +  for (;;)
> +    {
> +      int fd = open ("/dev/null", O_RDONLY, 0600);
> +      if (fd == -1)
> +	{
> +	  if (errno != EMFILE)
> +	    FAIL_EXIT1 ("create_temp_file: %m");

Wrong error message.

Maybe add TEST_VERIFY_EXIT (fd < max_fd) to the loop?  I believe the
setrlimit call will ensure that.

> diff --git a/manual/llio.texi b/manual/llio.texi
> index ceb18ac89a..777993d207 100644
> --- a/manual/llio.texi
> +++ b/manual/llio.texi
> @@ -321,6 +321,14 @@ The maximum number of file descriptors is controlled by the
>  @end table
>  @end deftypefun
>  
> +@deftypefun void closefrom (int @var{lowfd})
> +@standards{GNU, unistd.h}
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{@acsfd{}}}
> +
> +The function @code{closefrom} closes all file descriptors large or equal
> +then @var{lowfd}.  This function is similar to call @code{close} in specified
> +file descriptor range.
> +@end deftypefun

“larger than or equal to @var{lowfd}”

“@code{close} applied to the specified file descriptor range”

Please also mention that already-closed descriptors are ignored.

> diff --git a/sysdeps/unix/sysv/linux/closefrom.c b/sysdeps/unix/sysv/linux/closefrom.c
> new file mode 100644
> index 0000000000..ba98fccd39
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/closefrom.c
> +void
> +__closefrom (int lowfd)
> +{
> +  int l = MAX (0, lowfd);
> +
> +  int r = __close_range (l, ~0U, 0);
> +  if (r == 0)
> +    return;
> +
> +  if (!__closefrom_fallback (l))
> +    __fortify_fail ("closefrom failed to close a file descriptor");
> +}

This ignores EPERM.  I guess that's okay.

> diff --git a/sysdeps/unix/sysv/linux/closefrom_fallback.c b/sysdeps/unix/sysv/linux/closefrom_fallback.c
> new file mode 100644
> index 0000000000..78182bc5f0
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/closefrom_fallback.c
> @@ -0,0 +1,93 @@

> +/* Fallback code: iterates over /proc/self/fd, closing each file descriptor
> +   that fall on the criteria.  */
> +_Bool
> +__closefrom_fallback (int from)
> +{
> +  bool ret = false;
> +
> +  int dirfd = __open_nocancel (FD_TO_FILENAME_PREFIX, O_RDONLY | O_DIRECTORY,
> +			       0);
> +  if (dirfd == -1)
> +    {
> +      /* The closefrom should work even when process can't open new files.
> +	 In this case it loops over RLIMIT_NOFILE / rlim_cur until it frees
> +	 a file descriptor to iterate over /proc.  */
> +      if (errno == ENOENT)
> +	goto err;
> +
> +      int maxfd = __getdtablesize ();
> +      for (int i = from; i < maxfd; i++)
> +	if (__close_nocancel (i) == 0)
> +	  break;

This should check for errno != EBADF.  EINTR, EIO etc. are okay as well
because a descriptor has been released.

I wouldn't mind iterating up to INT_MAX, removing the __getdtablesize
call.  It may take a minute or two, but it removes some of the error
scenarios.

> +
> +      dirfd = __open_nocancel (FD_TO_FILENAME_PREFIX, O_RDONLY | O_DIRECTORY,
> +			       0);
> +      if (dirfd == -1)
> +	goto err;
> +    }
> +
> +  char buffer[1024];
> +  while (true)
> +    {
> +      ssize_t ret = __getdents64 (dirfd, buffer, sizeof (buffer));
> +      if (ret == -1)
> +        goto err;
> +      else if (ret == 0)
> +        break;
> +
> +      bool closed = false;

Maybe add a comment here about restarting on close?

> +      char *begin = buffer, *end = buffer + ret;
> +      while (begin != end)
> +	{
> +          unsigned short int d_reclen;
> +	  memcpy (&d_reclen, begin + offsetof (struct dirent64, d_reclen),
> +		  sizeof (d_reclen));
> +	  const char *dname = begin + offsetof (struct dirent64, d_name);
> +	  begin += d_reclen;
> +
> +	  if (dname[0] == '.')
> +	    continue;
> +
> +	  int fd = 0;
> +	  for (const char *s = dname; (unsigned int) (*s) - '0' < 10; s++)
> +	    fd = 10 * fd + (*s - '0');

Hmm.  I had to think a bit about it, but it seems okay whether *s is
signed or not.

> +	  if (fd == dirfd || fd < from)
> +	    continue;
> +
> +	  __close_nocancel (fd);

Maybe add a comment why we aren't adding an error here?

> +	  closed = true;
> +	}
> +
> +      if (closed)
> +        __lseek (dirfd, 0, SEEK_SET);
> +    }

Missing error check?

> +  ret = true;
> +err:
> +  __close_nocancel (dirfd);
> +  return ret;
> +}

The algorithm looks fine to me.  Thanks for adding the restart-on-close
part.

Thanks,
Florian