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

[Adhemerval Zanella]

On 22/12/2020 07:49, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/NEWS b/NEWS
>> index 9e829841f6..221eb4d98e 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -30,6 +30,10 @@ Major new features:
>>  
>>  * On Linux, the close_range function has been added.
>>  
>> +* The function closefrom has been adeed.  It closes all file descriptors
>> +  greater than given integer.  This function is a GNU extension, although it
>> +  also present in other systems.
> 
> Typo: “adeed”
> 

Ack.

> 
>> diff --git a/io/closefrom.c b/io/closefrom.c
>> new file mode 100644
>> index 0000000000..7e8f5a1ce5
>> --- /dev/null
>> +++ b/io/closefrom.c
>> @@ -0,0 +1,34 @@
> 
>> +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)
>> +      if (__close (i) == -1)
>> +        __fortify_fail ("closefrom failed to close a file descriptor");
>> +}
>> +weak_alias (__closefrom, closefrom)
> 
> Is __getdtablesize accurate on Hurd?  Then I guess this part is okay.

Not sure in fact, it seems so.

> 
> __close should probably be __close_nocancel_nostatus.  The error check
> is incorrect.

Yeah, I figure out after I sent it.  I will chance to __close_nocancel_nostatus.

> 
>> diff --git a/io/tst-closefrom.c b/io/tst-closefrom.c
>> new file mode 100644
>> index 0000000000..6eb97cec10
>> --- /dev/null
>> +++ b/io/tst-closefrom.c
> 
>> +  int fds[maximum_fd + 1];
> 
> Same issue as the other test?  Contiguous set of descriptors or not?

It assumes a contiguous, if you prefer I can remove the array and just use
a maximum value to hold the value.

> 
>> +#include <support/test-driver.c>
>> diff --git a/posix/unistd.h b/posix/unistd.h
>> index 32b8161619..43ddd516aa 100644
>> --- a/posix/unistd.h
>> +++ b/posix/unistd.h
>> @@ -352,6 +352,14 @@ extern __off64_t lseek64 (int __fd, __off64_t __offset, int __whence)
>>     __THROW.  */
>>  extern int close (int __fd);
>>  
>> +#ifdef __USE_GNU
>> +/* Close all open file descriptors greater than or equal to LOWFD.
>> +   Negative LOWFD is clamped to 0.
>> +
>> +   Similar to close, this functions is a cancellation point.  */
>> +extern void closefrom (int __lowfd);
>> +#endif
> 
> I think this function should not be a cancellation point, so you can add
> __THROW.
> 

Yeah, from close_range discussion it does seems a better alternative.

>> +weak_alias (__closefrom, closefrom)
>> diff --git a/sysdeps/unix/sysv/linux/closefrom_fallback.c b/sysdeps/unix/sysv/linux/closefrom_fallback.c
>> new file mode 100644
>> index 0000000000..48815941dd
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/closefrom_fallback.c
> 
>> +/* Fallback code: iterates over /proc/self/fd, closing each file descriptor
>> +   that fall on the criteria.  */
>> +_Bool
>> +__closefrom_fallback (int from)
>> +{
>> +  int dirfd = __open_nocancel (FD_TO_FILENAME_PREFIX, O_RDONLY | O_DIRECTORY,
>> +			       0);
>> +  if (dirfd == -1)
>> +    return false;
> 
> You could try to close a few descriptors if the error is ENOENT and try
> again.

Do you mean EMFILE and issue a close on [from, ...] and then try again?
I am not really fan of this heuristics, it might mitigate the failure if
limit of file-descriptors are reached, but it would really depend whether
if 'from' is close of the first open descriptor and the how many close
we issue.

> 
>> +  enum { buffer_size = 1024 };
>> +  char buffer[buffer_size];
>> +  bool ret = true;
> 
> buffer_size is only used once, so I think it's not needed.

Ack.

> 
>> +  while (true)
>> +    {
>> +      ssize_t ret = __getdents64 (dirfd, buffer, sizeof (buffer));
>> +      if (ret == -1)
>> +        goto err;
>> +      else if (ret == 0)
>> +        break;
>> +
>> +      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');
>> +
>> +	  if (fd == dirfd || fd < from)
>> +	    continue;
>> +
>> +	  if (__close_nocancel (fd) < 0)
>> +	    goto err;
>> +	}
>> +    }
>> +
>> +  ret = true;
>> +err:
>> +  __close_nocancel (dirfd);
>> +  return ret;
>> +}
> 
> I still think it's necessary to seek to position zero after exhausting
> the current buffer and some files have been closed.

Alright, this should only add some overhead; although based on various
current usages it really seems not required.

> 
> The error check for __close_nocancel seems superfluous.  It's possible
> that you get EBADF due to a race with another thread, and that does not
> seem sufficient reason to terminate the process?

Alright, I will remove the check.

> 
> Please also add a brief documentation to the manual (likewise for
> close_range).

Ack.